Skip to content

fix: maxBodySize can not short circuit on frameLengh #181

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.

Sign up for GitHub

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 3 commits into from
Aug 6, 2019
Merged

fix: maxBodySize can not short circuit on frameLengh #181

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cc0aced
fix: maxBodySize can not short circuit on frameLengh
Aug 3, 2019
a9bd851
Merge branch 'master' into max_body_size-final_frame
seebees Aug 3, 2019
72ee17a
Merge branch 'master' into max_body_size-final_frame
seebees Aug 4, 2019
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
2 changes: 1 addition & 1 deletion modules/decrypt-node/src/decrypt_stream.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export function decryptStream (
? new NodeDefaultCryptographicMaterialsManager(cmm)
: cmm

const parseHeaderStream = new ParseHeaderStream(cmm, { maxBodySize })
const parseHeaderStream = new ParseHeaderStream(cmm)
const verifyStream = new VerifyStream({ maxBodySize })
const decipherStream = getDecipherStream()

Expand Down
32 changes: 4 additions & 28 deletions modules/decrypt-node/src/parse_header_stream.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,9 @@ import { Transform } from 'stream' // eslint-disable-line no-unused-vars
import {
NodeAlgorithmSuite,
NodeMaterialsManager, // eslint-disable-line no-unused-vars
getDecryptionHelper,
needs
getDecryptionHelper
} from '@aws-crypto/material-management-node'
import { deserializeFactory, kdfInfo, ContentType } from '@aws-crypto/serialize'
import { deserializeFactory, kdfInfo } from '@aws-crypto/serialize'
import { VerifyInfo } from './verify_stream' // eslint-disable-line no-unused-vars

const toUtf8 = (input: Uint8Array) => Buffer
Expand All @@ -35,20 +34,12 @@ interface HeaderState {
buffer: Buffer
}

export interface ParseHeaderOptions {
maxBodySize?: number
}

export class ParseHeaderStream extends PortableTransformWithType {
Copy link
Contributor

Choose a reason for hiding this comment

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

Update https://github.com/awslabs/aws-encryption-sdk-javascript/blob/master/modules/decrypt-node/src/decrypt_stream.ts#L43 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I'm following...

https://github.com/awslabs/aws-encryption-sdk-javascript/pull/181/files#diff-a2a13589c6d6b1804302d56578057d38R43

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh weird, for some reason I missed that change. Ignore me.

private materialsManager!: NodeMaterialsManager
private _headerState: HeaderState
private _maxBodySize?: number
constructor (cmm: NodeMaterialsManager, { maxBodySize }: ParseHeaderOptions = {}) {
/* Precondition: ParseHeaderStream requires maxBodySize must be falsey or a number. */
needs(!maxBodySize || typeof maxBodySize === 'number', 'Unsupported MaxBodySize.')
constructor (cmm: NodeMaterialsManager) {
super()
Object.defineProperty(this, 'materialsManager', { value: cmm, enumerable: true })
Object.defineProperty(this, '_maxBodySize', { value: maxBodySize, enumerable: true })
this._headerState = {
buffer: Buffer.alloc(0)
}
Expand All @@ -67,22 +58,7 @@ export class ParseHeaderStream extends PortableTransformWithType {
const { rawHeader, headerIv, headerAuthTag } = headerInfo

const suite = new NodeAlgorithmSuite(algorithmSuite.id)
const { encryptionContext, encryptedDataKeys, contentType, frameLength } = messageHeader

/* Framed messages store the frame size in the header.
* It is easy to confirm here.
* For non-framed messages, the size is in the body header.
* The check in verify stream _should_ be adequate from a logical perspective.
* However, doing this check here allows framed messages to exit before the CMM is called.
* This means that decryption of the Encrypted Data Key is never even attempted.
*/
if (contentType === ContentType.FRAMED_DATA) {
/* Precondition: If maxBodySize was set I can not buffer a frame more data than maxBodySize.
* Before returning *any* cleartext, the stream **MUST** verify the decryption.
* This means that I must buffer the message until the AuthTag is reached.
*/
needs(!this._maxBodySize || this._maxBodySize >= frameLength, 'maxBodySize exceeded.')
}
const { encryptionContext, encryptedDataKeys } = messageHeader

this.materialsManager
.decryptMaterials({ suite, encryptionContext, encryptedDataKeys })
Expand Down
17 changes: 17 additions & 0 deletions modules/decrypt-node/test/decrypt.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,4 +83,21 @@ describe('decrypt', () => {
{ encoding: 'base64' }
)).to.rejectedWith(Error, 'Invalid Signature')
})

Copy link
Contributor

Choose a reason for hiding this comment

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

No unit tests for ParseHeaderStream?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Specifically, no. But implicitly, the stream test in this file will exercise just about everything.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless there is some reason why unit tests for ParseHeaderStream specifically would not be useful, we should add them.
Either in this PR, or (because the logic added in this PR seems to be sufficiently tested below) tracked in a new issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#188

it('can decrypt maxBodySize message with a single final frame.', async () => {
const { plaintext: test } = await decrypt(
fixtures.decryptKeyring(),
fixtures.base64Ciphertext4BytesWith4KFrameLength(),
{ encoding: 'base64', maxBodySize: 4 }
)
expect(test).to.deep.equal(Buffer.from('asdf'))
})

it('will not decrypt data that exceeds maxBodySize.', async () => {
return expect(decrypt(
fixtures.decryptKeyring(),
fixtures.base64Ciphertext4BytesWith4KFrameLength(),
{ encoding: 'base64', maxBodySize: 3 }
)).to.rejectedWith(Error, 'maxBodySize exceeded.')
})
})
4 changes: 4 additions & 0 deletions modules/decrypt-node/test/fixtures.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ export function invalidSignatureCiphertextAlgAes256GcmIv12Tag16HkdfSha384EcdsaP3
return 'AYADeJgnuW8vpQmi5QoqHIZWhjkAcAACABVhd3MtY3J5cHRvLXB1YmxpYy1rZXkAREFuWXRGRWV3Wm0rMjhLaElHcHg4UmhrYVVhTGNjSnB5ZjFud0lWUUZHbXlwZ3poSDJYZFNJQko0c0tpU0gzY2t6dz09AAZzaW1wbGUAB2NvbnRleHQAAQABawABawADAAAAAgAAAAAMAAAABQAAAAAAAAAAAAAAABqRZqpijpYGNM6P1L/78AUAAAABAAAAAAAAAAAAAAABIg1k1IeKV+CPUVBnpUkgyVUUZl7wAAAAAgAAAAAAAAAAAAAAAjl6P288VtjjKYeZA7mSeeJgjIUHbAAAAAMAAAAAAAAAAAAAAAO7OY+25yJkVcFvMMXn7VztyOhuIQoAAAAEAAAAAAAAAAAAAAAEG6jOHAz3NwyxgUjm5XFNMBx+2CCvAAAABQAAAAAAAAAAAAAABYRtGxVPUKbha73ay/kYrpl8Drik2gAAAAYAAAAAAAAAAAAAAAbosyHzP31p9EdOf3+dSa5gGfRW9e0AAAAHAAAAAAAAAAAAAAAHsulmBR4FQMbTk+00j5Fa/jD73/UJAAAACAAAAAAAAAAAAAAACMKgPZWTdDKzdPhXQDenInSRW/eOLgAAAAkAAAAAAAAAAAAAAAkdfSyNpBYk9XbFhf6DUnr2acw5lC4AAAAKAAAAAAAAAAAAAAAKnJpofr1UwwPy/+aqviMTrHXgOhM8AAAACwAAAAAAAAAAAAAAC9lvtW1lzA9RGUjnIGadlEhLxRC/FAAAAAwAAAAAAAAAAAAAAAyqJBaQEdmkOUX7uCki3Gh17YlQU3MAAAANAAAAAAAAAAAAAAANEK36ZE9VLiIj2X50N73UHEUtm0BbAAAADgAAAAAAAAAAAAAADkkr1fxL3qLbbC7OSDHqDnrBonOwxQAAAA8AAAAAAAAAAAAAAA8qcNFG+ofU3sOEZd8OXB/rkz0vDa8AAAAQAAAAAAAAAAAAAAAQ3KdsWJ/P8hF8aOhQdQP3v1KBDpB5AAAAEQAAAAAAAAAAAAAAEWyQGXefoGv9ZDfXUi93q+wUQGPzVwAAABIAAAAAAAAAAAAAABIDL/v5IY/z+s28FWzVo46vKNjOEeoAAAATAAAAAAAAAAAAAAATy1uc+McQfMJD8GrAJUaKlyTbXgFgAAAAFAAAAAAAAAAAAAAAFB6Sh2Po4oetBUwm1ABP9F9e1T70GAAAABUAAAAAAAAAAAAAABWm2oOg6agE6jzm3iDZ1brMSTHCOG8AAAAWAAAAAAAAAAAAAAAWsdIbfir5Dame3Uxkri54N2P7rqn6AAAAFwAAAAAAAAAAAAAAF6iPI1YW4fZzyL/355ZHBOLG3VPf1AAAABgAAAAAAAAAAAAAABj5Kjd5Twiu6bpb4o+jas0LRRJFH64AAAAZAAAAAAAAAAAAAAAZTf4xiUOtHeZmi+80M3Oay452R/rJAAAAGgAAAAAAAAAAAAAAGp+ET0LYxOX4JEL8gJudVVPW6qIv3AAAABsAAAAAAAAAAAAAABuTreBPGwJ2bftxQ6Kjwekfth4vWtsAAAAcAAAAAAAAAAAAAAAcdLoFVjR+yx4NVo1BSxv8Llya90EFAAAAHQAAAAAAAAAAAAAAHcFqEIL2wsYK36KQHyJqvJTiF/6nlQAAAB4AAAAAAAAAAAAAAB57QTT/UVRxBucxfhQRYeEU0mUeFxcAAAAfAAAAAAAAAAAAAAAfJyKwIcAURvMfN/Gd5MchygA20EYHAAAAIAAAAAAAAAAAAAAAILXRfQjIux8TeED/TdHHdLuaUEWWZgAAACEAAAAAAAAAAAAAACEi1SsfUozCXF0mCT/tHN8zVvSyWF4AAAAiAAAAAAAAAAAAAAAiFPt44yxRbwruA1F5YkYNokeDLmdiAAAAIwAAAAAAAAAAAAAAIwqdX86PI6IZgTs2SMHo4tLExClkIgAAACQAAAAAAAAAAAAAACQJGEuD6oBPBXU8iupaaNJFzEH/zKcAAAAlAAAAAAAAAAAAAAAlyQiA+1xRREA/qe5Djux6WaPEyUzhAAAAJgAAAAAAAAAAAAAAJqsZT21o1ikdiLkExG949WuTdw1mQQAAACcAAAAAAAAAAAAAACekCgcIX2x9/3zx982dDXfKUQSqARQAAAAoAAAAAAAAAAAAAAAocSNt9kEXLUF0Mydaj4MiBo1WrmGGAAAAKQAAAAAAAAAAAAAAKRHbcJJmpG367RxDInqlcBefk34RbgAAACoAAAAAAAAAAAAAACqmDdWYD/QVD9isxpCTm4KE+j6HKdMAAAArAAAAAAAAAAAAAAAreua98WTPIWH6dSAdzfYWPM9q9hoGAAAALAAAAAAAAAAAAAAALA+DQHkvoxKqVP3dmTQoM17QR4hz1gAAAC0AAAAAAAAAAAAAAC3TCjJBU0hDgBiC/bAHZe5T9CoMfTQAAAAuAAAAAAAAAAAAAAAujkLmjR2G1at5H5QHzKg/B2zNIH+mAAAALwAAAAAAAAAAAAAAL6+0F5aK0j3xqvgrsjmkzt7rZYUQQAAAADAAAAAAAAAAAAAAADDZMoeMElExOKgTTa0/gKqBPiRAqF4AAAAxAAAAAAAAAAAAAAAxbk1Qj+CqjC+gruT6bljBsQD5YTBVAAAAMgAAAAAAAAAAAAAAMhjQQjFR5A9Kn5ot/h4nqKrDTZJsNgAAADMAAAAAAAAAAAAAADO2SB3R/RrukhQx7/jxmjWiLknnnj0AAAA0AAAAAAAAAAAAAAA0wXykERn6CEIMhDCuLhUBmVn6fCu7AAAANQAAAAAAAAAAAAAANf7M3//4JJPLi+mmkKec2QrmuprdigAAADYAAAAAAAAAAAAAADadAVLY8PSrHytIi05tgse0HdyYVikAAAA3AAAAAAAAAAAAAAA3dj606o4y/YZw7gGHrD6JrGWQULV2AAAAOAAAAAAAAAAAAAAAOPgZF/TYVQogBfVMR6P4q5YWnSozUwAAADkAAAAAAAAAAAAAADl41/2WlW/Aq+EVJSHVH8eolMg7stIAAAA6AAAAAAAAAAAAAAA6IdfaZedkARnjm0CYxQhB28ljrigJAAAAOwAAAAAAAAAAAAAAO5PRn7sBV99dQJosnpj8Dy61bUW//QAAADwAAAAAAAAAAAAAADwkmUiXJJBJ4KvATXEeY1b2cOVPDOr/////AAAAPQAAAAAAAAAAAAAAPQAAAABAZDjPrFjtf/NJrKKMK2W9AGgwZgIxAN4h4KUn2VHZhxd/PQlZSmawzL1txgo79vsZjVhV15xqyMZLLcpNuNmK3hNHA83v+AIxAP0Sga/B1gZuyGmQK2cSnDdRIL6bmAzzeTiMcjRoJ6KrYRbLwg8mzmdQLgdvSoPt00=='
}

export function base64Ciphertext4BytesWith4KFrameLength () {
return 'AYADeEpqVoZzS6rBmbCKSa4acg4AXwABABVhd3MtY3J5cHRvLXB1YmxpYy1rZXkAREE4blREMk5rYzRDRXhoQTNlR0E3M1kyWWY2N2c4TDQySjlwL2UzNHo5RjdUR1hlT2pJZGR4TjEyeitUemk4Wmpxdz09AAEAAWsAAWsAAwAAAAIAAAAADAAAEAAAAAAAAAAAAAAAAABggOHpalFEq+ovk6c4Ugaj/////wAAAAEAAAAAAAAAAAAAAAEAAAAEDyVA5PCXHZo4sqQuqprmzWXwXUsAZzBlAjAY3vK9flPg8WEnoxIjPuhbj4o+bKUsJzbZgtDBaqIaBZYo8yt7JQB4vFyzb/PwucoCMQCz2fzhOQBAM63n56vBdIjBVqbNPyszIy0QjE8OJ/X8mV48VNi5wCF4u7I4MmK91N0='
}

export function base64Plaintext () {
return '3Ye0RVTIjYp9Yvi+81Dzq9h9gAUF6akM1mqTbPKMhmwgxTWuj6Wlf8UFUMG7zALPDpN77EleMS3dXUOGlr/nalmwXkBseEo+QxCJgeo6WMuB2xQHZqJT+0glM3mcl2FWwiQDZ9G84dYOW1KSDfiyISe9rTqARl0fmEnD1oB6zlP4cYg7+DDTxOOvw5RndoiOBZ+mLbZT9vHTsJkWB3HgFO06dFAtwSgjUAEaNWMjk04vIT+9SBvql6cOk+GLfUdkH33chNk22yKPF2UQ6+lvW0YqGODIfTBQXypPuuKYXJ3T583YxeiKoxuxZFpVNkg30r5cYPOYulINy+YrWQIbNFRP9Zk0CNkAJ7zsIMhQ8IXH+zG1bQmwh1RDGSAfZhmsR147Jsi6qty9Fe9O'
}
Expand Down