Bug 173646

Created attachment 313512 [details] WIP patch

Attachment 313512 [details] did not pass style-queue: ERROR: Source/WebCore/PAL/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 2 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 313520 [details] Patch Ready for review

Comment on attachment 313520 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313520&action=review Looks good to me. Please address the following comments: > Source/WebCore/PAL/pal/crypto/tasn1/Utilities.cpp:146 > + int ret = asn1_der_coding(root, elementName, nullptr, &length, nullptr); I didn't see any documentation that suggests this usage. Is this a hack? > Source/WebCore/PAL/pal/crypto/tasn1/Utilities.cpp:166 > +{ Maybe we just need the first one? Or we just don't need these two wrapper? > Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:424 > + if (!PAL::TASN1::writeElement(ecParameters, "", "namedCurve")) I didn't see any documentation that suggests this usage. Is the next step itself sufficient? > Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:446 > + auto data = PAL::TASN1::encodedData(ecParameters, ""); A little bit confused that why we don't need a name for the encoded data. > Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:469 > + auto result = PAL::TASN1::encodedData(spki, ""); Ditto.

Comment on attachment 313520 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313520&action=review >> Source/WebCore/PAL/pal/crypto/tasn1/Utilities.cpp:146 >> + int ret = asn1_der_coding(root, elementName, nullptr, &length, nullptr); > > I didn't see any documentation that suggests this usage. Is this a hack? No, this is a valid usage pattern, relying on ASN1_MEM_ERROR being returned, along with the length integer being set to the value of necessary bytes. As used in GnuTLS: https://gitlab.com/gnutls/gnutls/blob/master/lib/x509/x509.c#L2847 >> Source/WebCore/PAL/pal/crypto/tasn1/Utilities.cpp:166 >> +{ > > Maybe we just need the first one? Or we just don't need these two wrapper? OK, we should be able to use just one, and have that one already accept `const void*` as the data pointer parameter, instead of differentiating between const char* and const uint8_t*. >> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:424 >> + if (!PAL::TASN1::writeElement(ecParameters, "", "namedCurve")) > > I didn't see any documentation that suggests this usage. Is the next step itself sufficient? Yes. This call selects the `namedCurve` element in the `ECParameters` choice structure, the next one writes out the object identifier under it. Again the example of GnuTLS: https://gitlab.com/gnutls/gnutls/blob/master/lib/x509/key_encode.c#L263 In both calls, the default 0 value is used as the data size when calling the underlying asn1_write_value(). This differs from the GnuTLS usage as well as the documentation, but looking at the code the passed-in length value doesn't have any effect when writing out choice or object identifier values. I'll still drop the zero-length default and explicitly pass proper length values for every writeElement() call, following the documentation and GnuTLS code. >> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:446 >> + auto data = PAL::TASN1::encodedData(ecParameters, ""); > > A little bit confused that why we don't need a name for the encoded data. The specified name is searched for in the given asn1_node through the asn1_find_node() function. But if this is a null-escaped empty string, the original asn1_node is returned, meaning in this case that encoded data of the ecParameters structure is retrieved. http://git.savannah.gnu.org/gitweb/?p=libtasn1.git;a=blob;f=lib/parser_aux.c;h=976ab38f1863845c03ed5730d91ebe0113348f79;hb=HEAD#l152

Created attachment 313823 [details] Patch

Comment on attachment 313520 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313520&action=review >>> Source/WebCore/PAL/pal/crypto/tasn1/Utilities.cpp:146 >>> + int ret = asn1_der_coding(root, elementName, nullptr, &length, nullptr); >> >> I didn't see any documentation that suggests this usage. Is this a hack? > > No, this is a valid usage pattern, relying on ASN1_MEM_ERROR being returned, along with the length integer being set to the value of necessary bytes. > > As used in GnuTLS: > https://gitlab.com/gnutls/gnutls/blob/master/lib/x509/x509.c#L2847 OK. I guess I shouldn't only rely on its documentation. >>> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:424 >>> + if (!PAL::TASN1::writeElement(ecParameters, "", "namedCurve")) >> >> I didn't see any documentation that suggests this usage. Is the next step itself sufficient? > > Yes. This call selects the `namedCurve` element in the `ECParameters` choice structure, the next one writes out the object identifier under it. > > Again the example of GnuTLS: > https://gitlab.com/gnutls/gnutls/blob/master/lib/x509/key_encode.c#L263 > > In both calls, the default 0 value is used as the data size when calling the underlying asn1_write_value(). This differs from the GnuTLS usage as well as the documentation, but looking at the code the passed-in length value doesn't have any effect when writing out choice or object identifier values. I'll still drop the zero-length default and explicitly pass proper length values for every writeElement() call, following the documentation and GnuTLS code. OK. >>> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:446 >>> + auto data = PAL::TASN1::encodedData(ecParameters, ""); >> >> A little bit confused that why we don't need a name for the encoded data. > > The specified name is searched for in the given asn1_node through the asn1_find_node() function. But if this is a null-escaped empty string, the original asn1_node is returned, meaning in this case that encoded data of the ecParameters structure is retrieved. > > http://git.savannah.gnu.org/gitweb/?p=libtasn1.git;a=blob;f=lib/parser_aux.c;h=976ab38f1863845c03ed5730d91ebe0113348f79;hb=HEAD#l152 Got you.

Comment on attachment 313823 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313823&action=review r=me. Looks good to me. Please address the following comments. > Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:441 > + if (!PAL::TASN1::writeElement(spki, "algorithm.algorithm", "1.2.840.10045.2.1", 1)) Should we use some sort of constants for "1.2.840.10045.2.1"? It should be frequently used.

(In reply to Jiewen Tan from comment #8) > > Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:441 > > + if (!PAL::TASN1::writeElement(spki, "algorithm.algorithm", "1.2.840.10045.2.1", 1)) > > Should we use some sort of constants for "1.2.840.10045.2.1"? It should be > frequently used. Filed bug #174091 to gather these in a single location. I would prefer this to do as a cleanup after the SPKI and PKCS#8 patches land.

Comment on attachment 313823 [details] Patch Clearing flags on attachment: 313823 Committed r219064: <http://trac.webkit.org/changeset/219064>

All reviewed patches have been landed. Closing bug.