GNOME Bugzilla – Bug 745269
ECDSA fails test vectors for large curves, possible fix included
Last modified: 2015-03-02 16:56:00 UTC
xmlsec1 1.2.20 (openssl) There's an XML Security interoperability page from the W3C here: http://www.w3.org/TR/xmldsig-core1-interop/ which has approved test vectors here: http://www.w3.org/TR/xmldsig-core1-interop/interop/xmldsig11/oracle/ The smaller curves seem to work fine, but the largest one fails: # xmlsec1 --verify --pubkey-cert-der p256-cert.der signature-enveloping-p256_sha512.xml OK SignedInfo References (ok/all): 1/1 Manifests References (ok/all): 0/0 # xmlsec1 --verify --pubkey-cert-der p384-cert.der signature-enveloping-p384_sha384.xml OK SignedInfo References (ok/all): 1/1 Manifests References (ok/all): 0/0 # xmlsec1 --verify --pubkey-cert-der p521-cert.der signature-enveloping-p521_sha512.xml func=xmlSecOpenSSLEcdsaEvpVerify:file=signatures.c:line=1127:obj=unknown:subj=unknown:error=11:invalid size:xLen=66 > 64 func=xmlSecOpenSSLEvpSignatureVerify:file=signatures.c:line=486:obj=ecdsa-sha512:subj=EVP_VerifyFinal:error=4:crypto library function failed: func=xmlSecTransformVerifyNodeContent:file=transforms.c:line=1798:obj=ecdsa-sha512:subj=xmlSecTransformVerify:error=1:xmlsec library function failed: func=xmlSecDSigCtxVerify:file=xmldsig.c:line=385:obj=unknown:subj=xmlSecTransformVerifyNodeContent:error=1:xmlsec library function failed: Error: signature failed ERROR SignedInfo References (ok/all): 1/1 Manifests References (ok/all): 0/0 Error: failed to verify file "signature-enveloping-p521_sha512.xml" This looks like it dislikes the size of the signature. According to http://www.w3.org/TR/xmldsig-core1/#sec-ECDSA: "e.g. 32 for the P-256 curve and 66 for the P-521 curve" The size of the signature should be 66, not the 64 that xmlsec1 is expecting. In src/openssl/signatures.c:xmlSecOpenSSLEcdsaEvpVerify() (and xmlSecOpenSSLEcdsaEvpSign() too which is also broken for this curve) I see you're doing some checks of xLen against a preset value. I don't know if you just need to increase the value, or if the entire line of checking is wrong and extranious. You could elliminate all "group" and "order" code there if the size checks are unneccesary, and you can just trust the results from OpenSSL. In any case, this seems to fix the issues for me: --- src/openssl/signatures_old.c 2015-02-27 05:59:07.590884569 +0200 +++ src/openssl/signatures.c 2015-02-27 05:59:10.474884543 +0200 @@ -1040,16 +1040,6 @@ } xLen = BN_num_bytes(order); - if(xLen > (XMLSEC_OPENSSL_ECDSA_SIGNATURE_SIZE / 2)) { - xmlSecError(XMLSEC_ERRORS_HERE, - NULL, - NULL, - XMLSEC_ERRORS_R_INVALID_SIZE, - "xLen=%d > %d", - xLen, XMLSEC_OPENSSL_ECDSA_SIGNATURE_SIZE / 2); - goto done; - } - rSize = BN_num_bytes(s->r); sSize = BN_num_bytes(s->s); if((rSize > xLen) || (sSize > xLen)) { @@ -1123,15 +1113,6 @@ } xLen = BN_num_bytes(order); - if(xLen > (XMLSEC_OPENSSL_ECDSA_SIGNATURE_SIZE / 2)) { - xmlSecError(XMLSEC_ERRORS_HERE, - NULL, - NULL, - XMLSEC_ERRORS_R_INVALID_SIZE, - "xLen=%d > %d", - xLen, XMLSEC_OPENSSL_ECDSA_SIGNATURE_SIZE / 2); - goto done; - } if(siglen != xLen * 2) { xmlSecError(XMLSEC_ERRORS_HERE,
Fixed To ssh://aleksey@git.gnome.org/git/xmlsec 6b70034..3652b5e master -> master I didn't remove these checks completely as you suggested but instead just bumped the constant. It is used in other places too. Thanks for the bug report!
I looked at the fix you committed. While it fixes the bug for the secp521r1 curve it doesn't fix the bug for all larger curves. There's for example sect571r1 which would need to raise the limit from 66 to 72. The three curves in the test vectors supplied by the XML Security group are just samples of popular curves, the publication of the ECDSA algorithm (X9.62) mentions over a dozen potential curves. Raising it to 72 should fix the issue for all curves currently supported by OpenSSL. However, that might not be a long term solution, as larger curves could be added. The reason why I suggested removing the checks altogether is that appears to be more future proof, and I didn't fully understand the logic behind adding an upper limit. Although as you said, the number is used elsewhere, then I imagine a more complicated fix would be needed to future proof support for larger curves. In any case, thank you so much for the fast fix on this.
The key length is a user input since the key itself can come from the user (i.e. in the certificate). These checks are done to prevent bad things from happening if someone manages to send a really large key length somehow. It's just a good practice to validate ALL inputs :) Anyway, it doesn't need to be precise. I bumped it to 128 which should work for now To ssh://aleksey@git.gnome.org/git/xmlsec 3652b5e..d873c6d master -> master
I agree with validating all inputs, it just appears to me to be duplicating work already done. In xmlSecOpenSSLEcdsaEvpSign(), the check is done after a call to ECDSA_do_sign() which I imagine validates the data, and the return value from OpenSSL can be trusted. In xmlSecOpenSSLEcdsaEvpVerify(), you're already comparing the signature size elsewhere against the size the curve in question would generate as a separate check (siglen != xLen * 2), which again comes from OpenSSL. ECDSA_do_verify() would also verify whatever it is, again assuming OpenSSL can be trusted. For built in (named) curves, neither of these checks should be necessary (assuming it's not coming to workaround a bug in OpenSSL). Although I guess for explicit curves, which are not already approved by OpenSSL, you might want to prevent a potential issue with huge curves. In which case I'm not sure the checks are in the correct place. In both functions you already have the curve loaded, and in both functions you're calling EC_GROUP_get_order() with sizing functions which are already doing work of the same size that the following functions which would not be called upon size failure would be doing. If we wanted to ensure keys, signatures, and curves were of correct and sane sizes, shouldn't they be checked when they're loaded? (And perhaps already are.) I just don't see the benefit of doing these checks as part of signing or verification at the stages they're at. Then again, my knowledge of the intricacies of Elliptic Curve Cryptography is minimal, and the more I learn about how OpenSSL operates internally, the more I want to go shoot myself. I would appreciate any knowledge and rationale you can provide here, since as much as I despise the design of the XML Security Suite (and OpenSSL), we're forced to use it, and I really do want to learn whatever techniques are relavent for sane operation. Thank you!
Good points. I don't remember exact reasons for these checks. Given that the current constant is large enough to accommodate all the current curves I feel it is safer to keep these checks than to remove them. I put a comment in the code linking back to this discussion so next time this question pops up we can revisit this decision. Thanks again for your research!