From 742adcaa871b0a6e97469be3e3bd799c9b035879 Mon Sep 17 00:00:00 2001 From: seebees Date: Thu, 26 Mar 2020 14:35:13 -0700 Subject: [PATCH 1/5] fix: The final frame can not be larger than the Frame Length The final frame content length is stored in the body header. This means that while it should be related to the frame length in the header, it is not required. This enforces this requirement. As well as adding a test and test vector for a zero length final frame. --- modules/serialize/src/decode_body_header.ts | 2 + .../serialize/test/decode_body_header.test.ts | 58 ++++++++++++++++--- modules/serialize/test/fixtures.ts | 4 ++ 3 files changed, 56 insertions(+), 8 deletions(-) diff --git a/modules/serialize/src/decode_body_header.ts b/modules/serialize/src/decode_body_header.ts index aaa450fa9..d86ad3ffb 100644 --- a/modules/serialize/src/decode_body_header.ts +++ b/modules/serialize/src/decode_body_header.ts @@ -155,6 +155,8 @@ export function decodeFinalFrameBodyHeader (buffer: Uint8Array, headerInfo: Head needs(sequenceNumber > 0, 'Malformed sequenceNumber.') const iv = buffer.slice(readPos += 4, readPos += ivLength) const contentLength = dataView.getUint32(readPos) + /* Postcondition: The final frame should not be able to exceed the frameLength. */ + needs(headerInfo.messageHeader.frameLength >= contentLength, 'Final frame length exceeds frame length.') return { sequenceNumber, iv, diff --git a/modules/serialize/test/decode_body_header.test.ts b/modules/serialize/test/decode_body_header.test.ts index 5d003cd56..b7a9e0eba 100644 --- a/modules/serialize/test/decode_body_header.test.ts +++ b/modules/serialize/test/decode_body_header.test.ts @@ -102,7 +102,7 @@ describe('decodeFrameBodyHeader', () => { it('return final frame header', () => { const headerInfo = { messageHeader: { - frameLength: 99, + frameLength: 999, contentType: ContentType.FRAMED_DATA }, algorithmSuite: { @@ -206,7 +206,7 @@ describe('decodeFrameBodyHeader', () => { const buffer = concatBuffers(new Uint8Array(10), fixtures.finalFrameHeader()) const headerInfo = { messageHeader: { - frameLength: 99, + frameLength: 999, contentType: ContentType.FRAMED_DATA }, algorithmSuite: { @@ -297,7 +297,7 @@ describe('decodeFinalFrameBodyHeader', () => { it('return final frame header from readPos', () => { const headerInfo = { messageHeader: { - frameLength: 99, + frameLength: 999, contentType: ContentType.FRAMED_DATA }, algorithmSuite: { @@ -319,12 +319,36 @@ describe('decodeFinalFrameBodyHeader', () => { expect(test.tagLength).to.eql(16) expect(test.isFinalFrame).to.eql(true) expect(test.contentType).to.eql(ContentType.FRAMED_DATA) + expect(test.contentLength).to.eql(999) + }) + + it('The final frame can be 0 length.', () => { + const headerInfo = { + messageHeader: { + frameLength: 999, + contentType: ContentType.FRAMED_DATA + }, + algorithmSuite: { + ivLength: 12, + tagLength: 16 + } + } as any + const buffer = fixtures.finalFrameHeaderZeroBytes() + + const test = decodeFinalFrameBodyHeader(buffer, headerInfo, 0) + if (!test) throw new Error('failure') + expect(test.sequenceNumber).to.eql(1) + expect(test.iv).to.eql(fixtures.basicFrameIV()) + expect(test.tagLength).to.eql(16) + expect(test.isFinalFrame).to.eql(true) + expect(test.contentType).to.eql(ContentType.FRAMED_DATA) + expect(test.contentLength).to.eql(0) }) it('Precondition: The contentType must be FRAMED_DATA to be a Final Frame.', () => { const headerInfo = { messageHeader: { - frameLength: 99, + frameLength: 999, contentType: 'not FRAMED_DATA' }, algorithmSuite: { @@ -339,7 +363,7 @@ describe('decodeFinalFrameBodyHeader', () => { it('Precondition: decodeFinalFrameBodyHeader readPos must be within the byte length of the buffer given.', () => { const headerInfo = { messageHeader: { - frameLength: 99, + frameLength: 999, contentType: ContentType.FRAMED_DATA }, algorithmSuite: { @@ -356,7 +380,7 @@ describe('decodeFinalFrameBodyHeader', () => { it('Postcondition: sequenceEnd must be SEQUENCE_NUMBER_END.', () => { const headerInfo = { messageHeader: { - frameLength: 99, + frameLength: 999, contentType: ContentType.FRAMED_DATA }, algorithmSuite: { @@ -372,7 +396,7 @@ describe('decodeFinalFrameBodyHeader', () => { it('Postcondition: decodeFinalFrameBodyHeader sequenceNumber must be greater than 0.', () => { const headerInfo = { messageHeader: { - frameLength: 99, + frameLength: 999, contentType: ContentType.FRAMED_DATA }, algorithmSuite: { @@ -389,7 +413,7 @@ describe('decodeFinalFrameBodyHeader', () => { const frameHeader = fixtures.finalFrameHeader() const headerInfo = { messageHeader: { - frameLength: 99, + frameLength: 999, contentType: ContentType.FRAMED_DATA }, algorithmSuite: { @@ -403,6 +427,24 @@ describe('decodeFinalFrameBodyHeader', () => { expect(test).to.eql(false) } }) + + it('Postcondition: The final frame should not be able to exceed the frameLength.', () => { + const headerInfo = { + messageHeader: { + // The content length in this final frame is 999 + // So I set the frame length to less than this + frameLength: 99, + contentType: ContentType.FRAMED_DATA + }, + algorithmSuite: { + ivLength: 12, + tagLength: 16 + } + } as any + const buffer = fixtures.finalFrameHeader() + + expect(() => decodeFinalFrameBodyHeader(buffer, headerInfo, 0)).to.throw('Final frame length exceeds frame length.') + }) }) describe('decodeNonFrameBodyHeader', () => { diff --git a/modules/serialize/test/fixtures.ts b/modules/serialize/test/fixtures.ts index 1430f5fb6..adccd3caf 100644 --- a/modules/serialize/test/fixtures.ts +++ b/modules/serialize/test/fixtures.ts @@ -61,6 +61,10 @@ export function finalFrameHeader () { return new Uint8Array([255, 255, 255, 255, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 3, 231]) } +export function finalFrameHeaderZeroBytes () { + return new Uint8Array([255, 255, 255, 255, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0]) +} + export function invalidSequenceEndFinalFrameHeader () { return new Uint8Array([0, 255, 255, 255, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 3, 231]) } From 1f29bd6f4467b5a907c09476411aa1d565378c64 Mon Sep 17 00:00:00 2001 From: seebees Date: Fri, 27 Mar 2020 11:33:59 -0700 Subject: [PATCH 2/5] Update modules/serialize/src/decode_body_header.ts Co-Authored-By: Matt Bullock --- modules/serialize/src/decode_body_header.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/serialize/src/decode_body_header.ts b/modules/serialize/src/decode_body_header.ts index d86ad3ffb..2a46982e1 100644 --- a/modules/serialize/src/decode_body_header.ts +++ b/modules/serialize/src/decode_body_header.ts @@ -155,7 +155,7 @@ export function decodeFinalFrameBodyHeader (buffer: Uint8Array, headerInfo: Head needs(sequenceNumber > 0, 'Malformed sequenceNumber.') const iv = buffer.slice(readPos += 4, readPos += ivLength) const contentLength = dataView.getUint32(readPos) - /* Postcondition: The final frame should not be able to exceed the frameLength. */ + /* Postcondition: The final frame MUST NOT exceed the frameLength. */ needs(headerInfo.messageHeader.frameLength >= contentLength, 'Final frame length exceeds frame length.') return { sequenceNumber, From 9ed5d341ba7e03a36d6580fc857aff9215967f60 Mon Sep 17 00:00:00 2001 From: seebees Date: Fri, 27 Mar 2020 11:34:06 -0700 Subject: [PATCH 3/5] Update modules/serialize/test/decode_body_header.test.ts Co-Authored-By: Matt Bullock --- modules/serialize/test/decode_body_header.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/serialize/test/decode_body_header.test.ts b/modules/serialize/test/decode_body_header.test.ts index b7a9e0eba..e5b935f86 100644 --- a/modules/serialize/test/decode_body_header.test.ts +++ b/modules/serialize/test/decode_body_header.test.ts @@ -428,7 +428,7 @@ describe('decodeFinalFrameBodyHeader', () => { } }) - it('Postcondition: The final frame should not be able to exceed the frameLength.', () => { + it('Postcondition: The final frame MUST NOT exceed the frameLength.', () => { const headerInfo = { messageHeader: { // The content length in this final frame is 999 From 61302e49a32058fdf42f0072a556c6738ec92ef5 Mon Sep 17 00:00:00 2001 From: seebees Date: Fri, 27 Mar 2020 16:12:43 -0700 Subject: [PATCH 4/5] update the serialize path --- modules/encrypt-browser/src/encrypt.ts | 1 + .../encrypt-node/src/framed_encrypt_stream.ts | 11 ++- .../test/framed_encrypt_stream.test.ts | 89 ++++++++++++++++++- 3 files changed, 98 insertions(+), 3 deletions(-) diff --git a/modules/encrypt-browser/src/encrypt.ts b/modules/encrypt-browser/src/encrypt.ts index 39e51f3b5..366fbc42a 100644 --- a/modules/encrypt-browser/src/encrypt.ts +++ b/modules/encrypt-browser/src/encrypt.ts @@ -113,6 +113,7 @@ export async function encrypt ( /* The final frame has a variable length. * The value needs to be known, but should only be calculated once. * So I calculate how much of a frame I should have at the end. + * This value will NEVER be larger than the frameLength. */ const finalFrameLength = frameLength - ((numberOfFrames * frameLength) - plaintextLength) const bodyContent = [] diff --git a/modules/encrypt-node/src/framed_encrypt_stream.ts b/modules/encrypt-node/src/framed_encrypt_stream.ts index 5a039413d..2e5a991cf 100644 --- a/modules/encrypt-node/src/framed_encrypt_stream.ts +++ b/modules/encrypt-node/src/framed_encrypt_stream.ts @@ -190,11 +190,18 @@ type EncryptFrameInput = { export function getEncryptFrame (input: EncryptFrameInput): EncryptFrame { const { pendingFrame, messageHeader, getCipher, isFinalFrame } = input const { sequenceNumber, contentLength, content } = pendingFrame - const frameIv = serialize.frameIv(messageHeader.headerIvLength, sequenceNumber) + const { frameLength, contentType, messageId, headerIvLength } = messageHeader + /* Precondition: The content length MUST correlate with the frameLength. + * In the case of a regular frame, + * the content length must strictly equal the frame length. + * In the case of the final frame, + * it MUST not be larger than the frame length. + */ + needs(frameLength === contentLength || (isFinalFrame && frameLength >= contentLength), 'Final frame length exceeds frame length.') + const frameIv = serialize.frameIv(headerIvLength, sequenceNumber) const bodyHeader = Buffer.from(isFinalFrame ? finalFrameHeader(sequenceNumber, frameIv, contentLength) : frameHeader(sequenceNumber, frameIv)) - const { contentType, messageId } = messageHeader const contentString = aadUtility.messageAADContentString({ contentType, isFinalFrame }) const { buffer, byteOffset, byteLength } = aadUtility.messageAAD(messageId, contentString, sequenceNumber, contentLength) const cipher = getCipher(frameIv) diff --git a/modules/encrypt-node/test/framed_encrypt_stream.test.ts b/modules/encrypt-node/test/framed_encrypt_stream.test.ts index b244186ec..53bf2125d 100644 --- a/modules/encrypt-node/test/framed_encrypt_stream.test.ts +++ b/modules/encrypt-node/test/framed_encrypt_stream.test.ts @@ -18,7 +18,7 @@ import * as chai from 'chai' import chaiAsPromised from 'chai-as-promised' import 'mocha' -import { getFramedEncryptStream } from '../src/framed_encrypt_stream' +import { getFramedEncryptStream, getEncryptFrame } from '../src/framed_encrypt_stream' chai.use(chaiAsPromised) const { expect } = chai @@ -61,3 +61,90 @@ describe('getFramedEncryptStream', () => { expect(called).to.equal(true) }) }) + +describe('getEncryptFrame', () => { + it('can return an EncryptFrame', () => { + const input = { + pendingFrame: { + content: [Buffer.from([1, 2, 3, 4, 5])], + contentLength: 5, + sequenceNumber: 1 + }, + isFinalFrame: false, + getCipher: () => ({ setAAD: () => {} }) as any, + messageHeader: { + frameLength: 5, + contentType: 2, + messageId: Buffer.from([]), + headerIvLength: 12 as 12, + version: 1, + type: 12, + suiteId: 1, + encryptionContext: {}, + encryptedDataKeys: [] + } + } + const test1 = getEncryptFrame(input) + expect(test1.content).to.equal(input.pendingFrame.content) + expect(test1.isFinalFrame).to.equal(input.isFinalFrame) + + // Just a quick flip to make sure... + input.isFinalFrame = true + const test2 = getEncryptFrame(input) + expect(test2.content).to.equal(input.pendingFrame.content) + expect(test2.isFinalFrame).to.equal(input.isFinalFrame) + }) + + it('Precondition: The content length MUST correlate with the frameLength.', () => { + const inputFinalFrameToLarge = { + pendingFrame: { + content: [Buffer.from([1, 2, 3, 4, 5, 6])], + // This exceeds the frameLength below + contentLength: 6, + sequenceNumber: 1 + }, + isFinalFrame: true, + getCipher: () => ({ setAAD: () => {} }) as any, + messageHeader: { + frameLength: 5, + contentType: 2, + messageId: Buffer.from([]), + headerIvLength: 12 as 12, + version: 1, + type: 12, + suiteId: 1, + encryptionContext: {}, + encryptedDataKeys: [] + } + } + + expect(() => getEncryptFrame(inputFinalFrameToLarge)).to.throw('Final frame length exceeds frame length.') + + const inputFrame = { + pendingFrame: { + content: [Buffer.from([1, 2, 3, 4, 5])], + contentLength: 5, + sequenceNumber: 1 + }, + isFinalFrame: false, + getCipher: () => ({ setAAD: () => {} }) as any, + messageHeader: { + frameLength: 5, + contentType: 2, + messageId: Buffer.from([]), + headerIvLength: 12 as 12, + version: 1, + type: 12, + suiteId: 1, + encryptionContext: {}, + encryptedDataKeys: [] + } + } + + // Make sure that it must be equal as long as we are here... + inputFrame.pendingFrame.contentLength = 4 + expect(() => getEncryptFrame(inputFrame)).to.throw('Final frame length exceeds frame length.') + inputFrame.pendingFrame.contentLength = 6 + expect(() => getEncryptFrame(inputFrame)).to.throw('Final frame length exceeds frame length.') + }) +}) From 9a38ceadc8926dd0fa20c1d2eb0a88276ca938d2 Mon Sep 17 00:00:00 2001 From: seebees Date: Wed, 1 Apr 2020 14:26:10 -0700 Subject: [PATCH 5/5] update error message and tests Also, extent the check to the exact content length as long as we are here. --- modules/encrypt-node/src/framed_encrypt_stream.ts | 8 ++++---- modules/encrypt-node/test/framed_encrypt_stream.test.ts | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/modules/encrypt-node/src/framed_encrypt_stream.ts b/modules/encrypt-node/src/framed_encrypt_stream.ts index 2e5a991cf..3fac3d002 100644 --- a/modules/encrypt-node/src/framed_encrypt_stream.ts +++ b/modules/encrypt-node/src/framed_encrypt_stream.ts @@ -164,7 +164,7 @@ export function getFramedEncryptStream (getCipher: GetCipher, messageHeader: Mes /* Push the authTag onto the end. Yes, I am abusing the name. */ cipherContent.push(cipher.getAuthTag()) - needs(frameSize === frameLength || isFinalFrame, 'Malformed frame') + needs(frameSize === frameLength || (isFinalFrame && frameLength >= frameSize), 'Malformed frame') for (const cipherText of cipherContent) { if (!this.push(cipherText)) { @@ -193,11 +193,11 @@ export function getEncryptFrame (input: EncryptFrameInput): EncryptFrame { const { frameLength, contentType, messageId, headerIvLength } = messageHeader /* Precondition: The content length MUST correlate with the frameLength. * In the case of a regular frame, - * the content length must strictly equal the frame length. + * the content length MUST strictly equal the frame length. * In the case of the final frame, - * it MUST not be larger than the frame length. + * it MUST NOT be larger than the frame length. */ - needs(frameLength === contentLength || (isFinalFrame && frameLength >= contentLength), 'Final frame length exceeds frame length.') + needs(frameLength === contentLength || (isFinalFrame && frameLength >= contentLength), `Malformed frame length and content length: ${JSON.stringify({ frameLength, contentLength, isFinalFrame })}`) const frameIv = serialize.frameIv(headerIvLength, sequenceNumber) const bodyHeader = Buffer.from(isFinalFrame ? finalFrameHeader(sequenceNumber, frameIv, contentLength) diff --git a/modules/encrypt-node/test/framed_encrypt_stream.test.ts b/modules/encrypt-node/test/framed_encrypt_stream.test.ts index 41928c86e..7d5ed1004 100644 --- a/modules/encrypt-node/test/framed_encrypt_stream.test.ts +++ b/modules/encrypt-node/test/framed_encrypt_stream.test.ts @@ -117,7 +117,7 @@ describe('getEncryptFrame', () => { } } - expect(() => getEncryptFrame(inputFinalFrameToLarge)).to.throw('Final frame length exceeds frame length.') + expect(() => getEncryptFrame(inputFinalFrameToLarge)).to.throw('Malformed frame length and content length:') const inputFrame = { pendingFrame: { @@ -142,8 +142,8 @@ describe('getEncryptFrame', () => { // Make sure that it must be equal as long as we are here... inputFrame.pendingFrame.contentLength = 4 - expect(() => getEncryptFrame(inputFrame)).to.throw('Final frame length exceeds frame length.') + expect(() => getEncryptFrame(inputFrame)).to.throw('Malformed frame length and content length:') inputFrame.pendingFrame.contentLength = 6 - expect(() => getEncryptFrame(inputFrame)).to.throw('Final frame length exceeds frame length.') + expect(() => getEncryptFrame(inputFrame)).to.throw('Malformed frame length and content length:') }) })