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

Support for AES-256-CBC with W3C padding scheme ( http://www.w3.org/2001/04/xmlenc#aes256-cbc ) #368

Merged
merged 1 commit into from
Feb 6, 2017

Conversation

danielweck
Copy link
Contributor

Quoted from:
readium/readium-lcp-client#26 (comment)

"
IMO, we need to consider why W3C suddenly added the padding restriction in 2002.

There was no padding details before 2002 in the W3C encryption standard. But after publishing the possibility of the Padding Oracle Attack on the block cipher algorithm in 2002, W3C had instantly updated the specification as size of the padding bytes should not be described in the padding block except last byte.

Key point of the padding oracle is that the last encrypted block before the PKCS#7 padding could be easily cracked, if the decryption program provides unnecessary error codes related padding.

So I think that not allowing using the vulnerable PCKS#7 on both server and client side is the clear way for not only consistency of the specification, but also the security perspective.
"

CC @thkim2015 @drminside

@noloader
Copy link
Collaborator

noloader commented Jan 24, 2017

Thanks @danielweck.

The PR message was posted to the mailing list at PR 368: Support for AES-256-CBC with W3C padding scheme. It is a process formality.

Would you happen to know if test vectors exist? I'd like to add them independent of this patch. That is, I'll cut them in, and they don't need to be part of the patch.

@noloader
Copy link
Collaborator

noloader commented Jan 24, 2017

Sent to the W3C editors and authors.

On Tue, Jan 24, 2017 at 6:37 PM, Jeffrey Walton <...> wrote:
...
Wei Dai's Crypto++ received a request for W3C padding of block ciphers
at #368. I found
https://www.w3.org/Encryption/2002/02-xenc-interop.html, but its not
clear to me if it is the correct document or where the vectors are
located.

Would you be able to help with finding the test vectors for the padding scheme?

Thanks in advance,

@danielweck
Copy link
Contributor Author

@noloader thanks for your prompt feedback!
Let me clarify that the original authors for this CryptoPP patch are @thkim2015 / @drminside (all credits to them).
I thought I'd publish this PR as I am in the process of re-organising / cleaning-up the Crypto++ fork that's used in the Readium LCP project. Besides support for AES-256-CBC W3C padding scheme, we have minor XCode etc. project additions. Nothing else worth contributing back.
Cheers!
Dan

@danielweck
Copy link
Contributor Author

danielweck commented Jan 25, 2017

PS: also note that resources encrypted with AES-256-CBC PKCS#7 padding scheme can successfully be decrypted using the W3C-padding implementation proposed in this PR (but not vice-versa)

@noloader
Copy link
Collaborator

noloader commented Jan 25, 2017

@danielweck,

PS: also note that resources encrypted with AES-256-CBC PKCS#7 padding scheme can successfully be decrypted using the W3C-padding implementation proposed in this PR (but not vice-versa)

Ack thanks.

... have minor XCode etc. project additions

Let me know if we can help. As long as Xcode changes don't break other builds, we can probably incorporate them.

We test Apple operating systems like OS X and iOS, but WatchOS and AppleTV have gaps. I don't think they are being tested at the moment. Also see Issue 364.

You can also reach out off-list at noloader, gmail account.

@danielweck
Copy link
Contributor Author

@noloader
Copy link
Collaborator

noloader commented Feb 2, 2017

@danielweck,

Did test vectors materialize? Or, is there another (independent) implementation that I can generate them from?

(I never heard back from the W3C editors or the authors).

@danielweck
Copy link
Contributor Author

no exiting news from my end I'm afraid.

@thkim2015
Copy link

@noloader

Could you explain what you exactly mean the other party gets to view the arbitrary part if it is not set to specific value?
And I am sure what kind of risks there are, if it is disclosed as it is.

Although meaning 'Arbitrary' includes PKSCS#5(7) and always-specific value such as 0, however if the decryption module is not carefully operated on the padding information, it says some hacking program could get the decryption value without knowing key. It means the decryption module would be better not to know the values of the arbitrary part. So I think it could be the more risky if the decryption module has to check the arbitrary part should be specific value.

@noloader
Copy link
Collaborator

noloader commented Feb 6, 2017

@danielweck,

I'll be merging this today. There were no complaints from the folks on the user list.

We will need to tweak it by adding a W3C_PADDING path for encryption. The following test program:

int main(int argc, char* argv[])
{
	byte key[16] = {0}, iv[16] = {0};

	string str = "Hello World";
	string encrypted, recovered;
	
	CBC_Mode<AES>::Encryption enc(key, 16, iv);
	StringSource ss1(str, true, new StreamTransformationFilter(enc,
			new StringSink(encrypted), BlockPaddingSchemeDef::W3C_PADDING));
			
	CBC_Mode<AES>::Decryption dec(key, 16, iv);
	StringSource ss2(encrypted, true, new StreamTransformationFilter(dec,
			new StringSink(recovered), BlockPaddingSchemeDef::W3C_PADDING));
	
	cout << "Recovered: " << recovered << endl;

    return 0;
}

Causes an exception:

$ ./test.exe
terminate called after throwing an instance of 'CryptoPP::InvalidCiphertext'
  what():  StreamTransformationFilter: invalid W3C block padding found
Aborted (core dumped)

I believe its due to ONES_AND_ZEROS_PADDING.

Thanks again.

@noloader noloader merged commit 061f272 into weidai11:master Feb 6, 2017
@noloader
Copy link
Collaborator

noloader commented Feb 6, 2017

Also see Commit 8e088b48654f78cb.

@danielweck
Copy link
Contributor Author

Awesome additions @noloader ! :)

Credits to @thkim2015 @drminside for the original code in this pull request, and thanks to @edrlab for hosting the actual fork :) (which I will update as soon as the next official CryptoPP release comes out)

By the way, any idea when version 5.6.6 is likely to hit the release area?
https://github.com/weidai11/cryptopp/releases

@danielweck danielweck deleted the aes256cbc-w3c-padding-scheme branch February 6, 2017 15:44
danielweck added a commit to readium/readium-lcp-client that referenced this pull request Feb 6, 2017
…st (now merged to Crypto++ master branch, but not yet released), and additional original author's code, see: weidai11/cryptopp#368 (comment) Also see the following diff URL to see what changed in Readium LCP client lib's fork of CryptoPP at version 5.6.5 (added project files and W3C padding algo): edrlab/cryptopp@master...CRYPTOPP_5_6_5_CBC_W3C_PADDING#diff-a058f6967310c617f0e92efb3b5f1bd5
@noloader
Copy link
Collaborator

noloader commented Feb 6, 2017

By the way, any idea when version 5.6.6 is likely to hit the release area?

The next release will be 5.7 or 6.0. We inadvertently broke the ABI at 5.6.5, so we need a minor version bump to 5.7. We want to tweak the KDF interface so we can add modern derivation functions, and that will need a major version bump.

I don't know when it will be ready. We hope it will be in the next couple of months.

You might also consider pinging Jack Lloyd. He's the author of Botan. Its another good choice for C++ security libraries. Increasingly, I am using it for cross-validation.

@danielweck
Copy link
Contributor Author

Thanks for the pointers!

@noloader
Copy link
Collaborator

noloader commented May 9, 2017

Also see Commit 8a177c58e692.

noloader referenced this pull request May 9, 2017
This should have occurred with PR 368 or Commit 8e088b4
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