Skip to content

fix: plaintextLength must be enforced #213

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Sep 19, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions modules/encrypt-node/src/encrypt.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,15 @@ export async function encrypt (
plaintext: Buffer|Uint8Array|Readable|string|NodeJS.ReadableStream,
op: EncryptInput = {}
): Promise<EncryptOutput> {
const stream = encryptStream(cmm, op)
const { encoding } = op
if (plaintext instanceof Uint8Array) {
op.plaintextLength = plaintext.byteLength
} else if (typeof plaintext === 'string') {
plaintext = Buffer.from(plaintext, encoding)
op.plaintextLength = plaintext.byteLength
}

const stream = encryptStream(cmm, op)
const result: Buffer[] = []
let messageHeader: MessageHeader|false = false
stream
Expand All @@ -38,8 +44,6 @@ export async function encrypt (
// This will check both Uint8Array|Buffer
if (plaintext instanceof Uint8Array) {
stream.end(plaintext)
} else if (typeof plaintext === 'string') {
stream.end(Buffer.from(plaintext, encoding))
} else if (plaintext.readable) {
plaintext.pipe(stream)
} else {
Expand Down
2 changes: 1 addition & 1 deletion modules/encrypt-node/src/encrypt_stream.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ export function encryptStream (

wrappingStream.emit('MessageHeader', messageHeader)

const encryptStream = getFramedEncryptStream(getCipher, messageHeader, dispose)
const encryptStream = getFramedEncryptStream(getCipher, messageHeader, dispose, plaintextLength)
const signatureStream = new SignatureStream(getSigner)

pipeline(encryptStream, signatureStream)
Expand Down
16 changes: 14 additions & 2 deletions modules/encrypt-node/src/framed_encrypt_stream.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@

import {
serializeFactory, aadFactory,
MessageHeader // eslint-disable-line no-unused-vars
MessageHeader, // eslint-disable-line no-unused-vars
Maximum
} from '@aws-crypto/serialize'
// @ts-ignore
import { Transform as PortableTransform } from 'readable-stream'
Expand Down Expand Up @@ -49,11 +50,17 @@ const ioTick = () => new Promise(resolve => setImmediate(resolve))
const noop = () => {}
type ErrBack = (err?: Error) => void

export function getFramedEncryptStream (getCipher: GetCipher, messageHeader: MessageHeader, dispose: Function) {
export function getFramedEncryptStream (getCipher: GetCipher, messageHeader: MessageHeader, dispose: Function, plaintextLength?: number) {
let accumulatingFrame: AccumulatingFrame = { contentLength: 0, content: [], sequenceNumber: 1 }
let pathologicalDrain: Function = noop
const { frameLength } = messageHeader

/* Precondition: plaintextLength must be withing bounds.
* The Maximum.BYTES_PER_CACHED_KEY_LIMIT is set to be within Number.MAX_SAFE_INTEGER
* See serialize/identifiers.ts enum Maximum for more details.
*/
needs(!plaintextLength || (plaintextLength >= 0 && Maximum.BYTES_PER_CACHED_KEY_LIMIT >= plaintextLength), 'plaintextLength out of bounds.')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per offline discussion, Maximum.BYTES_PER_CACHED_KEY_LIMIT is the incorrect limit to apply here.
It should be Maximum.FRAME_COUNT * Maximum.FRAME_SIZE.
Because of the JS number limitations, this ends up needing to have the same numerical value, but we need to define it as a separate limit identifier.


/* Keeping the messageHeader, accumulatingFrame and pathologicalDrain private is the intention here.
* It is already unlikely that these values could be touched in the current composition of streams,
* but a different composition may change this.
Expand All @@ -63,6 +70,11 @@ export function getFramedEncryptStream (getCipher: GetCipher, messageHeader: Mes
_transform (chunk: Buffer, encoding: string, callback: ErrBack) {
const contentLeft = frameLength - accumulatingFrame.contentLength

/* Precondition: Must not process more than plaintextLength.
* The plaintextLength is the MAXIMUM value that can be encrypted.
*/
needs(!plaintextLength || (plaintextLength -= chunk.length) >= 0, 'Encrypted data exceeded plaintextLength.')

/* Check for early return (Postcondition): Have not accumulated a frame. */
if (contentLeft > chunk.length) {
// eat more
Expand Down
18 changes: 18 additions & 0 deletions modules/encrypt-node/test/framed_encrypt_stream.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,24 @@ describe('getFramedEncryptStream', () => {
expect(test._transform).is.a('function')
})

it('Precondition: plaintextLength must be withing bounds.', () => {
const getCipher: any = () => {}
expect(() => getFramedEncryptStream(getCipher, {} as any, () => {}, -1)).to.throw(Error, 'plaintextLength out of bounds.')
expect(() => getFramedEncryptStream(getCipher, {} as any, () => {}, Number.MAX_SAFE_INTEGER + 1)).to.throw(Error, 'plaintextLength out of bounds.')

/* Math is hard.
* I want to make sure that I don't have an errant off by 1 error.
*/
expect(() => getFramedEncryptStream(getCipher, {} as any, () => {}, Number.MAX_SAFE_INTEGER)).to.not.throw(Error)
})

it('Precondition: Must not process more than plaintextLength.', () => {
const getCipher: any = () => {}
const test = getFramedEncryptStream(getCipher, { } as any, () => {}, 8)

expect(() => test._transform(Buffer.from(Array(9)), 'binary', () => {})).to.throw(Error, 'Encrypted data exceeded plaintextLength.')
})

it('Check for early return (Postcondition): Have not accumulated a frame.', () => {
const getCipher: any = () => {}
const frameLength = 10
Expand Down