Skip to content

Commit 21050de

Browse files
committed
fix(oscore): address open review items from PR #397
- server: hoist _oscoreOnly out of the oscoreContexts gate so the flag is honoured when contexts are added at runtime via addOscoreContext() - agent: drop unsolicited plaintext datagrams in oscoreOnly mode for peers that have no registered context (defense in depth) - middlewares: drop the (oscore as any).clearRebootRecovery() cast; the method is publicly typed in coap-oscore ≥ 2.2.1 - middlewares: wrap parseOscoreOption() in try/catch so malformed OSCORE options surface as 4.01 Unauthorized rather than bubbling through the middleware chain as a 5.00 - oscore_helpers: reject reserved high flag bits (0xe0) and PIV lengths 6 and 7 per RFC 8613 §6.1 - oscore: give _pendingEchoNonces entries a 30s TTL so peers that never reply to an Echo challenge can't accumulate in the map; matched storePendingEcho/clearPendingEcho/getPendingEcho accessors accordingly - helpers: introduce ECHO_OPTION constant (RFC 9175 option 252) and use it everywhere instead of the magic string '252' - agent: switch import crypto = require('crypto') to ESM-style import for consistency with the rest of the codebase - README: collapse the two-require server example into a single require from 'coap' (OscoreContextStatus is re-exported) - package.json: revert version bump to 1.4.2 (matches master), bump coap-oscore floor to ^2.2.2 (latest published) Adds 4 new tests covering the changes above: - oscoreOnly applied via addOscoreContext() at runtime - oscoreOnly agent silently drops unsolicited inbound plaintext - malformed OSCORE option (reserved PIV length) → 4.01 - pending Echo nonce evicted after TTL All 24 OSCORE tests pass; 492 in the wider suite (IPv6 tests excluded — broken in sandbox, unaffected by these changes).
1 parent a84ee04 commit 21050de

8 files changed

Lines changed: 203 additions & 20 deletions

File tree

README.md

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -178,8 +178,7 @@ req.end()
178178

179179
```js
180180
const coap = require('coap')
181-
const { OSCORE, SecurityContextManager } = require('coap')
182-
const { OscoreContextStatus } = require('coap-oscore')
181+
const { OSCORE, OscoreContextStatus, SecurityContextManager } = require('coap')
183182

184183
const contexts = new SecurityContextManager()
185184

lib/agent.ts

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
* See the included LICENSE file for more details.
77
*/
88

9-
import crypto = require('crypto')
9+
import crypto from 'crypto'
1010
import { Socket, createSocket } from 'dgram'
1111
import { AgentOptions, CoapRequestParams, Block, Parameters } from '../models/models'
1212
import { EventEmitter } from 'events'
@@ -21,7 +21,7 @@ import { SegmentedTransmission } from './segmentation'
2121
import { parseBlockOption } from './block'
2222
import { AddressInfo } from 'net'
2323
import { parameters, createParameters } from './parameters'
24-
import { hasOscoreOption } from './oscore_helpers'
24+
import { hasOscoreOption, ECHO_OPTION } from './oscore_helpers'
2525

2626
interface OscoreRsinfo extends AddressInfo {
2727
oscore?: boolean
@@ -94,6 +94,11 @@ class Agent extends EventEmitter {
9494
this._sock.on('message', (msg, rsinfo) => {
9595
const oscoreCtx = this._getOscoreContext(rsinfo.address, rsinfo.port)
9696
if (oscoreCtx == null) {
97+
if (this._oscoreOnly) {
98+
// Drop unsolicited plaintext from peers with no context
99+
// when the agent is in OSCORE-only mode.
100+
return
101+
}
97102
this._handlePlainMessage(msg, rsinfo)
98103
return
99104
}
@@ -420,7 +425,7 @@ class Agent extends EventEmitter {
420425

421426
// Echo auto-retry for OSCORE peers
422427
if (packet.code === '4.01' && req != null && this._getOscoreContext(rsinfo.address, rsinfo.port) != null) {
423-
const echoOpt = getOption(packet.options, '252')
428+
const echoOpt = getOption(packet.options, ECHO_OPTION)
424429
if (echoOpt != null) {
425430
// Limit Echo retries to prevent infinite loops from malicious servers
426431
const retryCount = (req as any)._echoRetries ?? 0
@@ -441,7 +446,7 @@ class Agent extends EventEmitter {
441446
}
442447
}
443448
}
444-
retryReq.setOption('252', echoOpt)
449+
retryReq.setOption(ECHO_OPTION, echoOpt)
445450

446451
;(retryReq as any)._echoRetries = retryCount + 1
447452

@@ -485,7 +490,7 @@ class Agent extends EventEmitter {
485490
response = new ObserveStream(packet, rsinfo, outSocket)
486491
if (rsinfo.oscore === true) {
487492
(response as ObserveStream)._disableFiltering = true
488-
;(response as ObserveStream).oscoreProtected = true
493+
response.oscoreProtected = true
489494
}
490495
response.on('close', () => {
491496
this._tkToReq.delete(packet.token.toString('hex'))

lib/middlewares.ts

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import { parse, generate, ParsedPacket } from 'coap-packet'
1111
import { Socket } from 'dgram'
1212
import { or, isOption, getOption } from './helpers'
1313
import { MiddlewareParameters } from '../models/models'
14-
import { getOscoreOptionValue, parseOscoreOption } from './oscore_helpers'
14+
import { getOscoreOptionValue, parseOscoreOption, ECHO_OPTION } from './oscore_helpers'
1515

1616
type middlewareCallback = (nullOrError: null | Error) => void
1717

@@ -133,7 +133,18 @@ export function oscoreDecryptRequest (request: MiddlewareParameters, next: middl
133133
return next(null)
134134
}
135135

136-
const { kid, kidContext } = parseOscoreOption(oscoreOptValue)
136+
let kid: Buffer | null
137+
let kidContext: Buffer | null
138+
try {
139+
({ kid, kidContext } = parseOscoreOption(oscoreOptValue))
140+
} catch {
141+
// Malformed OSCORE option (reserved bits, PIV length out of range, etc.)
142+
request.server._sendError(
143+
Buffer.from('Unauthorized'),
144+
request.rsinfo, request.packet, '4.01'
145+
)
146+
return
147+
}
137148
const ctxMgr = request.server._oscoreContextManager
138149

139150
if (ctxMgr == null || kid == null) {
@@ -179,7 +190,7 @@ export function oscoreDecryptRequest (request: MiddlewareParameters, next: middl
179190
if (decrypted != null) {
180191
try {
181192
const innerPacket = parse(decrypted)
182-
const echoVal = getOption(innerPacket.options, '252' as any)
193+
const echoVal = getOption(innerPacket.options, ECHO_OPTION)
183194
if (Buffer.isBuffer(echoVal)) {
184195
innerEcho = echoVal
185196
}
@@ -197,8 +208,7 @@ export function oscoreDecryptRequest (request: MiddlewareParameters, next: middl
197208
crypto.timingSafeEqual(innerEcho, storedNonce)
198209
) {
199210
// Echo verified — complete the reboot recovery
200-
// Note: clearRebootRecovery() is added in the concurrent node-oscore update
201-
;(oscore as any).clearRebootRecovery()
211+
oscore.clearRebootRecovery()
202212
ctxMgr.clearPendingEcho(kid, kidContext ?? undefined)
203213

204214
// Continue processing with the decrypted inner message
@@ -227,7 +237,7 @@ export function oscoreDecryptRequest (request: MiddlewareParameters, next: middl
227237
ack: request.packet?.confirmable === true,
228238
messageId: request.packet?.messageId,
229239
token: request.packet?.token,
230-
options: [{ name: '252' as any, value: echoNonce }]
240+
options: [{ name: ECHO_OPTION as any, value: echoNonce }]
231241
})
232242

233243
oscore.encode(innerResponse)

lib/oscore.ts

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,13 @@ import type { OSCORE } from 'coap-oscore'
1616
*/
1717
export class SecurityContextManager extends EventEmitter {
1818
private static readonly MAX_TOKEN_BINDINGS = 10000
19+
// Echo challenges only need to live long enough for the client to
20+
// round-trip the nonce back; if the client never replies, the entry
21+
// is evicted so the map can't grow unbounded.
22+
private static readonly PENDING_ECHO_TTL_MS = 30_000
1923
private _contexts: Map<string, OSCORE>
2024
private _tokenToContext: Map<string, OSCORE>
21-
private _pendingEchoNonces: Map<string, Buffer>
25+
private _pendingEchoNonces: Map<string, { nonce: Buffer, timer: NodeJS.Timeout }>
2226

2327
constructor () {
2428
super()
@@ -98,26 +102,42 @@ export class SecurityContextManager extends EventEmitter {
98102

99103
/**
100104
* Store a pending Echo nonce for a given security context.
105+
* The entry self-evicts after PENDING_ECHO_TTL_MS to prevent the map
106+
* from accumulating entries when peers never reply to the challenge.
101107
*/
102108
storePendingEcho (recipientId: Buffer, idContext: Buffer | undefined, nonce: Buffer): void {
103109
const key = this._toKey(recipientId, idContext)
104-
this._pendingEchoNonces.set(key, nonce)
110+
const existing = this._pendingEchoNonces.get(key)
111+
if (existing != null) {
112+
clearTimeout(existing.timer)
113+
}
114+
const timer = setTimeout(() => {
115+
this._pendingEchoNonces.delete(key)
116+
}, SecurityContextManager.PENDING_ECHO_TTL_MS)
117+
if (typeof timer.unref === 'function') {
118+
timer.unref()
119+
}
120+
this._pendingEchoNonces.set(key, { nonce, timer })
105121
}
106122

107123
/**
108124
* Retrieve the pending Echo nonce for a given security context.
109125
*/
110126
getPendingEcho (recipientId: Buffer, idContext: Buffer | undefined): Buffer | undefined {
111127
const key = this._toKey(recipientId, idContext)
112-
return this._pendingEchoNonces.get(key)
128+
return this._pendingEchoNonces.get(key)?.nonce
113129
}
114130

115131
/**
116132
* Clear the pending Echo nonce for a given security context.
117133
*/
118134
clearPendingEcho (recipientId: Buffer, idContext: Buffer | undefined): void {
119135
const key = this._toKey(recipientId, idContext)
120-
this._pendingEchoNonces.delete(key)
136+
const entry = this._pendingEchoNonces.get(key)
137+
if (entry != null) {
138+
clearTimeout(entry.timer)
139+
this._pendingEchoNonces.delete(key)
140+
}
121141
}
122142

123143
private _toKey (recipientId: Buffer, idContext?: Buffer): string {

lib/oscore_helpers.ts

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,16 @@ const OSCORE_OPTION_NUMBER = 9
1313
const FLAG_KID = 0x08
1414
const FLAG_KID_CTX = 0x10
1515
const FLAG_PIV_MASK = 0x07
16+
// Bits 5-7 of the OSCORE option flag byte are reserved per RFC 8613 §6.1
17+
// and MUST be 0 in a valid option.
18+
const FLAG_RESERVED_MASK = 0xe0
19+
// PIV (Partial IV) length is encoded in the low 3 bits; values 6 and 7 are
20+
// reserved per RFC 8613 §6.1.
21+
const MAX_PIV_LEN = 5
22+
23+
// RFC 9175 Echo option number (used as the OSCORE reboot-recovery challenge
24+
// per RFC 8613 Appendix B.1.2).
25+
export const ECHO_OPTION = '252'
1626

1727
export interface OscoreOptionFields {
1828
kid: Buffer | null
@@ -53,7 +63,14 @@ export function parseOscoreOption (value: Buffer): OscoreOptionFields {
5363
let offset = 0
5464
const flags = value[offset++]
5565

66+
if ((flags & FLAG_RESERVED_MASK) !== 0) {
67+
throw new Error('Malformed OSCORE option: reserved flag bits set')
68+
}
69+
5670
const pivLen = flags & FLAG_PIV_MASK
71+
if (pivLen > MAX_PIV_LEN) {
72+
throw new Error('Malformed OSCORE option: PIV length reserved')
73+
}
5774
let piv: Buffer | null = null
5875
if (pivLen > 0) {
5976
if (offset + pivLen > value.length) {

lib/server.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,9 +152,10 @@ class CoAPServer extends EventEmitter {
152152
this._parameters.sendAcksForNonConfirmablePackets
153153
}
154154

155+
this._oscoreOnly = this._options.oscoreOnly ?? false
156+
155157
if (this._options.oscoreContexts != null) {
156158
this._oscoreContextManager = this._options.oscoreContexts
157-
this._oscoreOnly = this._options.oscoreOnly ?? false
158159
this._middlewares.push(oscoreDecryptRequest)
159160
}
160161

package.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "coap",
3-
"version": "1.6.0",
3+
"version": "1.4.2",
44
"description": "A CoAP library for node modelled after 'http'",
55
"main": "dist/index.js",
66
"types": "dist/index.d.ts",
@@ -57,7 +57,7 @@
5757
"bl": "^6.0.12",
5858
"@types/readable-stream": "^4.0.14",
5959
"capitalize": "^2.0.4",
60-
"coap-oscore": "^2.2.1",
60+
"coap-oscore": "^2.2.2",
6161
"coap-packet": "^1.1.1",
6262
"debug": "^4.3.5",
6363
"fastseries": "^2.0.0",

test/oscore.ts

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1078,4 +1078,135 @@ describe('OSCORE', function () {
10781078
})
10791079
}).timeout(2000)
10801080
})
1081+
1082+
describe('oscoreOnly applied via addOscoreContext', function () {
1083+
it('should reject plaintext after a context is added at runtime', function (done) {
1084+
const port = nextPort()
1085+
const { server: serverOscore } = createOscorePair()
1086+
1087+
// Server is created with oscoreOnly:true but no contexts yet
1088+
const server = trackServer(createServer({ oscoreOnly: true }))
1089+
server.on('request', () => {
1090+
done(new Error('Should not receive request — oscoreOnly should reject plaintext'))
1091+
})
1092+
1093+
server.listen(port, () => {
1094+
server.addOscoreContext(serverOscore, Buffer.from('01', 'hex'))
1095+
1096+
const plainAgent = trackAgent(new Agent({ type: 'udp4' }))
1097+
const req = request({
1098+
hostname: '127.0.0.1',
1099+
port,
1100+
pathname: '/test',
1101+
agent: plainAgent
1102+
})
1103+
req.on('response', (res) => {
1104+
expect(res.code).to.equal('4.01')
1105+
done()
1106+
})
1107+
req.end()
1108+
})
1109+
})
1110+
})
1111+
1112+
describe('oscoreOnly agent drops inbound plaintext', function () {
1113+
it('should silently drop unsolicited plaintext from peers with no context', function (done) {
1114+
const agent = trackAgent(new Agent({ type: 'udp4', oscoreOnly: true }))
1115+
const agentSock: any = (agent as any)._sock
1116+
let errored = false
1117+
agent.on('error', () => { errored = true })
1118+
1119+
agentSock.bind(0, '127.0.0.1', () => {
1120+
const agentPort = agentSock.address().port
1121+
const sender = dgram.createSocket('udp4')
1122+
const datagram = generate({
1123+
code: '2.05',
1124+
payload: Buffer.from('plain'),
1125+
messageId: 1,
1126+
token: Buffer.alloc(0)
1127+
})
1128+
sender.send(datagram, agentPort, '127.0.0.1', () => {
1129+
sender.close()
1130+
// Give the agent a tick to (not) emit anything
1131+
setTimeout(() => {
1132+
try {
1133+
expect(errored).to.equal(false)
1134+
done()
1135+
} catch (e) {
1136+
done(e as Error)
1137+
}
1138+
}, 100)
1139+
})
1140+
})
1141+
}).timeout(2000)
1142+
})
1143+
1144+
describe('PIV validation', function () {
1145+
it('should reject OSCORE options with reserved PIV length', function (done) {
1146+
const port = nextPort()
1147+
const { server: serverOscore } = createOscorePair()
1148+
const contexts = new SecurityContextManager()
1149+
contexts.addContext(serverOscore, Buffer.from('01', 'hex'))
1150+
1151+
const server = trackServer(createServer({ oscoreContexts: contexts }))
1152+
server.on('request', () => {
1153+
done(new Error('Should not receive request — malformed PIV must be rejected'))
1154+
})
1155+
1156+
server.listen(port, () => {
1157+
// Hand-craft a CoAP packet with an OSCORE option whose flag byte
1158+
// encodes PIV length 6 (reserved per RFC 8613 §6.1).
1159+
const oscoreValue = Buffer.concat([
1160+
Buffer.from([0x06]), // flags: pivLen=6
1161+
Buffer.alloc(6) // arbitrary PIV bytes
1162+
])
1163+
const packet = generate({
1164+
code: '0.01',
1165+
confirmable: true,
1166+
messageId: 1,
1167+
token: Buffer.from([0xaa]),
1168+
options: [{ name: 'OSCORE' as any, value: oscoreValue }]
1169+
})
1170+
const client = dgram.createSocket('udp4')
1171+
client.on('message', (msg) => {
1172+
try {
1173+
const reply = parse(msg)
1174+
expect(reply.code).to.equal('4.01')
1175+
client.close()
1176+
done()
1177+
} catch (e) {
1178+
client.close()
1179+
done(e as Error)
1180+
}
1181+
})
1182+
client.send(packet, port, '127.0.0.1')
1183+
})
1184+
}).timeout(2000)
1185+
})
1186+
1187+
describe('pending Echo nonce TTL', function () {
1188+
it('should evict stored Echo nonces after the TTL elapses', function (done) {
1189+
// Shortcut the TTL for the test by monkey-patching the static
1190+
// (readonly is TS-only; the value is captured at store time).
1191+
const originalTtl = (SecurityContextManager as any).PENDING_ECHO_TTL_MS
1192+
;(SecurityContextManager as any).PENDING_ECHO_TTL_MS = 50
1193+
1194+
const contexts = new SecurityContextManager()
1195+
const recipientId = Buffer.from('01', 'hex')
1196+
const nonce = Buffer.from('cafebabe', 'hex')
1197+
contexts.storePendingEcho(recipientId, undefined, nonce)
1198+
expect(contexts.getPendingEcho(recipientId, undefined)).to.deep.equal(nonce)
1199+
1200+
setTimeout(() => {
1201+
try {
1202+
expect(contexts.getPendingEcho(recipientId, undefined)).to.equal(undefined)
1203+
done()
1204+
} catch (e) {
1205+
done(e as Error)
1206+
} finally {
1207+
;(SecurityContextManager as any).PENDING_ECHO_TTL_MS = originalTtl
1208+
}
1209+
}, 150)
1210+
}).timeout(2000)
1211+
})
10811212
})

0 commit comments

Comments
 (0)