Skip to content
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

AES256 CBC W3C padding scheme (instead of PKCS#7) #111

Closed
wants to merge 2 commits into from

Conversation

drminside
Copy link

@drminside drminside commented Jan 23, 2017

In reaction to:
#18 (comment)

It write pad size in the last of pad, and another pad block is not initialized.

@danielweck danielweck changed the title Pad algorithm change recommendation from W3C AES256 CBC W3C padding scheme (instead of PKCS#7) Jan 23, 2017
@danielweck
Copy link
Member

danielweck commented Jan 23, 2017

Client implementation: readium/readium-lcp-client#26

@@ -74,7 +74,9 @@ func (r *paddedReader) Read(buf []byte) (int, error) {
func (r *paddedReader) pad(buf []byte) (i int, err error) {
capacity := cap(buf)
for i = 0; capacity > 0 && r.left > 0; i++ {
buf[i] = r.count
if capacity == 1 && r.left == 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

You assume here that the other padding bytes (not the last one set to r.count) are "arbitrary" ?
How do you know that they are not 0, or leaked for a part of memory ?
I think that we have to set all the padding bytes.

@drminside
Copy link
Author

In the doc page in golang official web site, "make" function is described "it returns an initialized (not zeroed) value of type T". Thus we expected that other pad block except last one will has any value, not zero.
Accept your comment, RemiBauzac, we add a code for generate random number for setting pad block. Thanks for your comment.

danielweck added a commit that referenced this pull request Jan 27, 2017
…ader object, based on DRM-Inside's #111 Pull Request (updated unit tests too)
@danielweck
Copy link
Member

Thank you very much!
I created a new Pull Request which supersedes this one:
#113
I updated the unit tests, and added some comments to explain PKCS#7 vs. W3C padding scheme for AES-256-CBC

@danielweck danielweck closed this Jan 27, 2017
@danielweck
Copy link
Member

@drminside there was a bug in your original code (fixed in the new PR):
if capacity == 1 && r.left == 1
should be
if r.left == 1
as revealed by unit test TestOneBlock:
https://github.com/readium/readium-lcp-server/blob/master/crypto/pad_test.go#L9
(4-byte source, 6-byte block size, 12-byte reader buffer => capacity does not reach 1 because 6 extra bytes in reader buffer are ignored)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants