Re: [PATCH] X.509: Support parse long form of length octets in Authority Key Identifier
Hi David,
First, thanks for your review and comments!
於 三，2013-02-13 於 13:47 +，David Howells 提到：
> Lee, Chun-Yi wrote:
>
> > Signed-off-by: Lee, Chun-Yi
>
> Without the comma, please.
>
> > + /* Short Form length */
> > + if (vlen <= 127) {
> > +
> > + if (v[1] != vlen - 2 ||
>
> There's an unnecessary blank line there. I would also move the comment inside
> the if-body.
I will move the comment to if-body, thanks!
>
> > + int sub = v[1] - 0x80;
>
> I recommend you use an unsigned int or size_t variable.
Yes, using size_t is better, will change it.
>
> Also, you should use ASN1_INDEFINITE_LENGTH rather than writing 0x80 and 127.
Thanks, will change it.
>
> > + v += 4;
> > + key_len = v[3];
>
> Are you sure that is correct? You altered v before doing v[3]. I would stick
> with key_len = vlen - 4.
Sorry for my stupid mistake!
It causes by I changed the key_len's value assignment from before check
length of vlen to after, the reason is just want source code "looks
better".
I will fix it, appreciate for your correction!
>
> > + /* calculate the length from subsequent octet */
>
> "... octets".
>
I will fix the typo, thanks!
> > + seq_len |= v[2 + i];
>
> Add 2 to v before entering the loop.
Thanks for your suggestion, it's a better style, I will change it.
>
> > + /* check vlen should not less then length of keyid */
>
> vlen should be exactly equal to key id, shouldn't it? Leastways, that's what
> you're checking...
hm... no, the value of vlen should be equal or bigger then the length of
key id, because the AuthorityKeyIdentifier SEQUENCE may also includes
authorityCertIssuer and authorityCertSerialNumber but not just
keyIdentifier. So, it's possible:
a. vlen = length of keyIdentifier (short form)
b. vlen = length of authorityCertIssuer + length of authorityCertSerialNumber
(long form)
c. vlen = length of keyIdentifier + length of authorityCertIssuer + length of
authorityCertSerialNumber (long form)
But, you are right, the first element in AuthorityKeyIdentifier SEQUENCE
maybe is not keyIdentifier, the authorityCertIssuer is also possible the
first element when there have no keyIdentifier.
I will direct remove the comment for avoid the confusing.
>
> > + v += (4 + sub);
> > + key_len = v[3 + sub];
>
> Again, this doesn't look right. Besides, you should be able to work out
> key_len from vlen, subtracting the metadata size.
>
> David
>
Sorry for my stupid mistake again, I will fix it!
Thanks a lot!
Joey Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] X.509: Support parse long form of length octets in Authority Key Identifier
Hi David,
First, thanks for your review and comments!
於 三，2013-02-13 於 13:47 +，David Howells 提到：
Lee, Chun-Yi joeyli.ker...@gmail.com wrote:
Signed-off-by: Lee, Chun-Yi j...@suse.com
Without the comma, please.
+ /* Short Form length */
+ if (vlen = 127) {
+
+ if (v[1] != vlen - 2 ||
There's an unnecessary blank line there. I would also move the comment inside
the if-body.
I will move the comment to if-body, thanks!
+ int sub = v[1] - 0x80;
I recommend you use an unsigned int or size_t variable.
Yes, using size_t is better, will change it.
Also, you should use ASN1_INDEFINITE_LENGTH rather than writing 0x80 and 127.
Thanks, will change it.
+ v += 4;
+ key_len = v[3];
Are you sure that is correct? You altered v before doing v[3]. I would stick
with key_len = vlen - 4.
Sorry for my stupid mistake!
It causes by I changed the key_len's value assignment from before check
length of vlen to after, the reason is just want source code looks
better.
I will fix it, appreciate for your correction!
+ /* calculate the length from subsequent octet */
... octets.
I will fix the typo, thanks!
+ seq_len |= v[2 + i];
Add 2 to v before entering the loop.
Thanks for your suggestion, it's a better style, I will change it.
+ /* check vlen should not less then length of keyid */
vlen should be exactly equal to key id, shouldn't it? Leastways, that's what
you're checking...
hm... no, the value of vlen should be equal or bigger then the length of
key id, because the AuthorityKeyIdentifier SEQUENCE may also includes
authorityCertIssuer and authorityCertSerialNumber but not just
keyIdentifier. So, it's possible:
a. vlen = length of keyIdentifier (short form)
b. vlen = length of authorityCertIssuer + length of authorityCertSerialNumber
(long form)
c. vlen = length of keyIdentifier + length of authorityCertIssuer + length of
authorityCertSerialNumber (long form)
But, you are right, the first element in AuthorityKeyIdentifier SEQUENCE
maybe is not keyIdentifier, the authorityCertIssuer is also possible the
first element when there have no keyIdentifier.
I will direct remove the comment for avoid the confusing.
+ v += (4 + sub);
+ key_len = v[3 + sub];
Again, this doesn't look right. Besides, you should be able to work out
key_len from vlen, subtracting the metadata size.
David
Sorry for my stupid mistake again, I will fix it!
Thanks a lot!
Joey Lee
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] X.509: Support parse long form of length octets in Authority Key Identifier
Lee, Chun-Yi wrote:
> Signed-off-by: Lee, Chun-Yi
Without the comma, please.
> + /* Short Form length */
> + if (vlen <= 127) {
> +
> + if (v[1] != vlen - 2 ||
There's an unnecessary blank line there. I would also move the comment inside
the if-body.
> + int sub = v[1] - 0x80;
I recommend you use an unsigned int or size_t variable.
Also, you should use ASN1_INDEFINITE_LENGTH rather than writing 0x80 and 127.
> + v += 4;
> + key_len = v[3];
Are you sure that is correct? You altered v before doing v[3]. I would stick
with key_len = vlen - 4.
> + /* calculate the length from subsequent octet */
"... octets".
> + seq_len |= v[2 + i];
Add 2 to v before entering the loop.
> + /* check vlen should not less then length of keyid */
vlen should be exactly equal to key id, shouldn't it? Leastways, that's what
you're checking...
> + v += (4 + sub);
> + key_len = v[3 + sub];
Again, this doesn't look right. Besides, you should be able to work out
key_len from vlen, subtracting the metadata size.
David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] X.509: Support parse long form of length octets in Authority Key Identifier
Lee, Chun-Yi joeyli.ker...@gmail.com wrote:
Signed-off-by: Lee, Chun-Yi j...@suse.com
Without the comma, please.
+ /* Short Form length */
+ if (vlen = 127) {
+
+ if (v[1] != vlen - 2 ||
There's an unnecessary blank line there. I would also move the comment inside
the if-body.
+ int sub = v[1] - 0x80;
I recommend you use an unsigned int or size_t variable.
Also, you should use ASN1_INDEFINITE_LENGTH rather than writing 0x80 and 127.
+ v += 4;
+ key_len = v[3];
Are you sure that is correct? You altered v before doing v[3]. I would stick
with key_len = vlen - 4.
+ /* calculate the length from subsequent octet */
... octets.
+ seq_len |= v[2 + i];
Add 2 to v before entering the loop.
+ /* check vlen should not less then length of keyid */
vlen should be exactly equal to key id, shouldn't it? Leastways, that's what
you're checking...
+ v += (4 + sub);
+ key_len = v[3 + sub];
Again, this doesn't look right. Besides, you should be able to work out
key_len from vlen, subtracting the metadata size.
David
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] X.509: Support parse long form of length octets in Authority Key Identifier
於 三，2013-02-06 於 13:08 +0800，Lee, Chun-Yi 提到：
> Per X.509 spec in 4.2.1.1 section, the structure of Authority Key
> Identifier Exception is:
^^ Extension
Sorry for my typo, I will send patch again.
Thanks
Joey Lee
>
>AuthorityKeyIdentifier ::= SEQUENCE {
> keyIdentifier [0] KeyIdentifier OPTIONAL,
> authorityCertIssuer [1] GeneralNamesOPTIONAL,
> authorityCertSerialNumber [2] CertificateSerialNumber OPTIONAL }
>
>KeyIdentifier ::= OCTET STRING
>
> When a certificate also provides
> authorityCertIssuer and authorityCertSerialNumber then the length of
> AuthorityKeyIdentifier SEQUENCE is likely to long form format.
> e.g.
>The example certificate demos/tunala/A-server.pem in openssl source:
>
> X509v3 Authority Key Identifier:
> keyid:49:FB:45:72:12:C4:CC:E1:45:A1:D3:08:9E:95:C4:2C:6D:55:3F:17
> DirName:/C=NZ/L=Wellington/O=Really Irresponsible Authorisation Authority
> (RIAA)/OU=Cert-stamping/CN=Jackov al-Trades/emailAddress=none@fake.domain
> serial:00
>
> Current parsing rule of OID_authorityKeyIdentifier only take care the
> short form format, it causes load certificate to modsign_keyring fail:
>
> [ 12.061147] X.509: Extension: 47
> [ 12.075121] MODSIGN: Problem loading in-kernel X.509 certificate (-74)
>
> So, this patch add the parsing rule for support long form format against
> Authority Key Identifier.
>
> Cc: David Howells
> Cc: Rusty Russell
> Cc: Josh Boyer
> Cc: Randy Dunlap
> Cc: Herbert Xu
> Cc: "David S. Miller"
> Signed-off-by: Lee, Chun-Yi
> ---
> crypto/asymmetric_keys/x509_cert_parser.c | 56
> 1 files changed, 48 insertions(+), 8 deletions(-)
>
> diff --git a/crypto/asymmetric_keys/x509_cert_parser.c
> b/crypto/asymmetric_keys/x509_cert_parser.c
> index 7fabc4c..7f6a152 100644
> --- a/crypto/asymmetric_keys/x509_cert_parser.c
> +++ b/crypto/asymmetric_keys/x509_cert_parser.c
> @@ -373,6 +373,9 @@ int rsa_extract_mpi(void *context, size_t hdrlen,
> return 0;
> }
>
> +/* The keyIdentifier in AuthorityKeyIdentifier SEQUENCE is tag(CONT,PRIM,0)
> */
> +#define SEQ_TAG_KEYID (ASN1_CONT << 6)
> +
> /*
> * Process certificate extensions that are used to qualify the certificate.
> */
> @@ -407,21 +410,58 @@ int x509_process_extension(void *context, size_t hdrlen,
> }
>
> if (ctx->last_oid == OID_authorityKeyIdentifier) {
> + size_t key_len;
> +
> /* Get hold of the CA key fingerprint */
> if (vlen < 5)
> return -EBADMSG;
> - if (v[0] != (ASN1_SEQ | (ASN1_CONS << 5)) ||
> - v[1] != vlen - 2 ||
> - v[2] != (ASN1_CONT << 6) ||
> - v[3] != vlen - 4)
> +
> + /* Authority Key Identifier must be a Constructed SEQUENCE */
> + if (v[0] != (ASN1_SEQ | (ASN1_CONS << 5)))
> return -EBADMSG;
> - v += 4;
> - vlen -= 4;
>
> - f = kmalloc(vlen * 2 + 1, GFP_KERNEL);
> + /* Authority Key Identifier is not indefinite length */
> + if (unlikely(vlen == ASN1_INDEFINITE_LENGTH))
> + return -EBADMSG;
> +
> + /* Short Form length */
> + if (vlen <= 127) {
> +
> + if (v[1] != vlen - 2 ||
> + v[2] != SEQ_TAG_KEYID ||
> + v[3] != vlen - 4)
> + return -EBADMSG;
> +
> + v += 4;
> + key_len = v[3];
> + } else {
> + /* Long Form length */
> + size_t seq_len = 0;
> + int sub = v[1] - 0x80;
> +
> + if (sub > 2)
> + return -EBADMSG;
> +
> + /* calculate the length from subsequent octet */
> + for (i = 0; i < sub; i++) {
> + seq_len <<= 8;
> + seq_len |= v[2 + i];
> + }
> +
> + /* check vlen should not less then length of keyid */
> + if (seq_len != vlen - 2 - sub ||
> + v[2 + sub] != SEQ_TAG_KEYID ||
> + v[3 + sub] > vlen - 4 - sub)
> + return -EBADMSG;
> +
> + v += (4 + sub);
> + key_len = v[3 + sub];
> + }
> +
> + f = kmalloc(key_len * 2 + 1, GFP_KERNEL);
> if (!f)
> return -ENOMEM;
> - for (i = 0; i < vlen; i++)
> + for (i = 0; i < key_len; i++)
> sprintf(f + i * 2, "%02x", v[i]);
> pr_debug("authority %s\n", f);
> ctx->cert->authority = f;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Re: [PATCH] X.509: Support parse long form of length octets in Authority Key Identifier
於 三，2013-02-06 於 13:08 +0800，Lee, Chun-Yi 提到：
Per X.509 spec in 4.2.1.1 section, the structure of Authority Key
Identifier Exception is:
^^ Extension
Sorry for my typo, I will send patch again.
Thanks
Joey Lee
AuthorityKeyIdentifier ::= SEQUENCE {
keyIdentifier [0] KeyIdentifier OPTIONAL,
authorityCertIssuer [1] GeneralNamesOPTIONAL,
authorityCertSerialNumber [2] CertificateSerialNumber OPTIONAL }
KeyIdentifier ::= OCTET STRING
When a certificate also provides
authorityCertIssuer and authorityCertSerialNumber then the length of
AuthorityKeyIdentifier SEQUENCE is likely to long form format.
e.g.
The example certificate demos/tunala/A-server.pem in openssl source:
X509v3 Authority Key Identifier:
keyid:49:FB:45:72:12:C4:CC:E1:45:A1:D3:08:9E:95:C4:2C:6D:55:3F:17
DirName:/C=NZ/L=Wellington/O=Really Irresponsible Authorisation Authority
(RIAA)/OU=Cert-stamping/CN=Jackov al-Trades/emailAddress=none@fake.domain
serial:00
Current parsing rule of OID_authorityKeyIdentifier only take care the
short form format, it causes load certificate to modsign_keyring fail:
[ 12.061147] X.509: Extension: 47
[ 12.075121] MODSIGN: Problem loading in-kernel X.509 certificate (-74)
So, this patch add the parsing rule for support long form format against
Authority Key Identifier.
Cc: David Howells dhowe...@redhat.com
Cc: Rusty Russell ru...@rustcorp.com.au
Cc: Josh Boyer jwbo...@redhat.com
Cc: Randy Dunlap rdun...@xenotime.net
Cc: Herbert Xu herb...@gondor.apana.org.au
Cc: David S. Miller da...@davemloft.net
Signed-off-by: Lee, Chun-Yi j...@suse.com
---
crypto/asymmetric_keys/x509_cert_parser.c | 56
1 files changed, 48 insertions(+), 8 deletions(-)
diff --git a/crypto/asymmetric_keys/x509_cert_parser.c
b/crypto/asymmetric_keys/x509_cert_parser.c
index 7fabc4c..7f6a152 100644
--- a/crypto/asymmetric_keys/x509_cert_parser.c
+++ b/crypto/asymmetric_keys/x509_cert_parser.c
@@ -373,6 +373,9 @@ int rsa_extract_mpi(void *context, size_t hdrlen,
return 0;
}
+/* The keyIdentifier in AuthorityKeyIdentifier SEQUENCE is tag(CONT,PRIM,0)
*/
+#define SEQ_TAG_KEYID (ASN1_CONT 6)
+
/*
* Process certificate extensions that are used to qualify the certificate.
*/
@@ -407,21 +410,58 @@ int x509_process_extension(void *context, size_t hdrlen,
}
if (ctx-last_oid == OID_authorityKeyIdentifier) {
+ size_t key_len;
+
/* Get hold of the CA key fingerprint */
if (vlen 5)
return -EBADMSG;
- if (v[0] != (ASN1_SEQ | (ASN1_CONS 5)) ||
- v[1] != vlen - 2 ||
- v[2] != (ASN1_CONT 6) ||
- v[3] != vlen - 4)
+
+ /* Authority Key Identifier must be a Constructed SEQUENCE */
+ if (v[0] != (ASN1_SEQ | (ASN1_CONS 5)))
return -EBADMSG;
- v += 4;
- vlen -= 4;
- f = kmalloc(vlen * 2 + 1, GFP_KERNEL);
+ /* Authority Key Identifier is not indefinite length */
+ if (unlikely(vlen == ASN1_INDEFINITE_LENGTH))
+ return -EBADMSG;
+
+ /* Short Form length */
+ if (vlen = 127) {
+
+ if (v[1] != vlen - 2 ||
+ v[2] != SEQ_TAG_KEYID ||
+ v[3] != vlen - 4)
+ return -EBADMSG;
+
+ v += 4;
+ key_len = v[3];
+ } else {
+ /* Long Form length */
+ size_t seq_len = 0;
+ int sub = v[1] - 0x80;
+
+ if (sub 2)
+ return -EBADMSG;
+
+ /* calculate the length from subsequent octet */
+ for (i = 0; i sub; i++) {
+ seq_len = 8;
+ seq_len |= v[2 + i];
+ }
+
+ /* check vlen should not less then length of keyid */
+ if (seq_len != vlen - 2 - sub ||
+ v[2 + sub] != SEQ_TAG_KEYID ||
+ v[3 + sub] vlen - 4 - sub)
+ return -EBADMSG;
+
+ v += (4 + sub);
+ key_len = v[3 + sub];
+ }
+
+ f = kmalloc(key_len * 2 + 1, GFP_KERNEL);
if (!f)
return -ENOMEM;
- for (i = 0; i vlen; i++)
+ for (i = 0; i key_len; i++)
sprintf(f + i * 2, %02x, v[i]);
pr_debug(authority %s\n, f);
ctx-cert-authority = f;
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a
