Re: [openssl-dev] [ #3665] Bug report and a patch forOpenSSL 1.0.1l (and 1.0.1k)

2015-01-20 Thread Uri Blumenthal via RT
Steve Henson correctly pointed out that to change ASN1_TYPE_cmp() may not be 
appropriate, as there could be cases when null pointer (absent list) means 
something different from list being NULL.

To address that concern, I've made sure the workaround applies only to the case 
when two algorithms are compared, based on RFC 5754 and 5280 that state that 
AlgorithmIdentifier parameter list can be absent or represented as ASN.1 NULL - 
and implementations MUST accept both cases.

This patch also addresses the case when ASN1_TYPE_cmp(a, b) is called with a == 
b == NULL. Current implementation thinks that 0 != 0, which is not correct.

Please find attached my updated patch patch-null-absent.diff”:

--- crypto/asn1/a_type.c.~1~ 2015-01-15 09:43:14.0 -0500
+++ crypto/asn1/a_type.c 2015-01-20 22:57:48.0 -0500
@@ -117,6 +117,8 @@
  int result = -1;

+ if (!a  !b) return 0; /* both null-pointers = both absent/equal */
  if (!a || !b || a-type != b-type) return -1;

  switch (a-type)
--- crypto/asn1/x_algor.c.~1~ 2015-01-15 09:43:14.0 -0500
+++ crypto/asn1/x_algor.c 2015-01-20 23:00:54.0 -0500
@@ -151,5 +151,12 @@
  return rv;
  if (!a-parameter  !b-parameter)
  return 0;
+ if ((!a-parameter  b-parameter
+  b-parameter-type == V_ASN1_NULL)
+ ||
+ (!b-parameter  a-parameter
+  a-parameter-type == V_ASN1_NULL)
+ )
+   return 0;
  return ASN1_TYPE_cmp(a-parameter, b-parameter);

Description: Binary data
openssl-dev mailing list
To unsubscribe:

Re: [openssl-dev] [ #3665] Bug report and a patch forOpenSSL 1.0.1l (and 1.0.1k)

2015-01-19 Thread Rob Stradling via RT
On 19/01/15 14:47, Stephen Henson via RT wrote:
 On Mon Jan 19 14:40:32 2015, steve wrote:

 The problem is that the two fields containing the signature algorithm
 do not match.

 The current 'x509' utility can't show this difference (I have an option I'm
 testing which will).

Steve, while you're there...

I've been caught out a few times in the past because the 'x509' utility 
displays the outer signature algorithm in the place where it should 
display the inner signature algorithm.  This is fine when they match, 
but it's rather unhelpful when they don't match!

Please consider this trivial patch.  Thanks.

diff --git a/crypto/asn1/t_x509.c b/crypto/asn1/t_x509.c
index 89115c7..97abd51 100644
--- a/crypto/asn1/t_x509.c
+++ b/crypto/asn1/t_x509.c
@@ -168,7 +168,7 @@ int X509_print_ex(BIO *bp, X509 *x, unsigned long 
nmflags, unsigned long cflag)

 if(!(cflag  X509_FLAG_NO_SIGNAME))
-   if(X509_signature_print(bp, x-sig_alg, NULL) = 0)
+   if(X509_signature_print(bp, ci-signature, NULL) = 0)
 goto err;
  #if 0
 if (BIO_printf(bp,%8sSignature Algorithm: ,) = 0)

Rob Stradling
Senior Research  Development Scientist
COMODO - Creating Trust Online

openssl-dev mailing list
To unsubscribe:

Re: [openssl-dev] [ #3665] Bug report and a patch forOpenSSL 1.0.1l (and 1.0.1k)

2015-01-19 Thread Uri Blumenthal via RT
Steve, you’ve made several good points. But please consider:

 The problem is that the two fields containing the signature algorithm do not 

But they do according to RFC 5754 - I interpret it as declaring that in this 
particular case (optional parameters of AlgorithmIdentifier) NULL is equivalent 
to (the same as) absent. In fact, RFC 5754 (page 4) states
that the correct encoding is omitting the empty list altogether, but some uses 
defined their encoding
as NULL, and it’s OK. It reveals some history of this duality:

The two alternatives arise from the loss of the OPTIONAL associated with the  
algorithm identifier parameters when the 1988 syntax for
   AlgorithmIdentifier was translated into the 1997 syntax.  Later, the
   OPTIONAL was recovered via a defect report, but by then many people
   thought that algorithm parameters were mandatory.  Because of this
   history, some implementations encode parameters as a NULL element
   while others omit them entirely.

 Now whether the same algorithm identifier means identical or the same 
 meaning is debatable. Currently it goes with the former and if that is taken 
 to be the case the certificate presented is encoded incorrectly. 

I see your point. However I don’t find it stated anywhere that the same object 
in a certificate must be encoded the same way if it occurs more than once. 
Thus, it may be a silly and bad idea to encode AlgorithmIdentifier one way 
first, and the other way next time - but it doesn’t appear prohibited.

Is 2*2 the same as 4? :-)  Or better, is “2” the same as “two”? :-)

 The question is whether an OpenSSL workaround should be added to tolerate 

Considering the growing popularity of Apple platforms, my humble opinion is yes 
- a workaround should be added to tolerate this legal (and ugly) case.

 Before this change OpenSSL completely ignored the signature field (so it 
 could contain anything) and only used the signatureAlgorithm field. 

Exactly. And there are a few certificates deployed that are legal in every 
aspect of the published RFCs that 1.0.1k patch invalidates unnecessarily.

 In general the ASN.1 NULL value and absent can be very different depending on 
 the ASN.1 structure being represented and empty can have a third distinct 
 meaning. Example: imagine an OPTIONAL field where NULL represents a status 
 value of some sort. In that case an absent field would indicate no status 
 available while a NULL would indicate a specific status. So in general 
 changing ASN1_TYPE_cmp in the proposed fashion is not a good idea. 

Yes I see your point, and I agree with you. In general it isn’t a good idea to 
change ASN1_TYPE_cmp(). That means my patch isn’t good as is, and should be 

 In the very specific case of a certificate and certain digest algorithms 
 absent and NULL do have the same meaning. 

Exactly. I understand you as stating that a patch like what I’m proposing 
should be limited to this specific narrow case.

 It seems odd that an implementation having decided it should represent an 
 algorithm in one way for one field should then decide to represent an 
 identical algorithm in a different way for another.

Yes it is very odd, and I can only speculate as to how that implementation came 
about to doing that weird and strange thing. But it is not illegal, because 
both representations are legal, and there is no explicit requirement to stick 
with one representation, and no prohibition to mix representations. I’m not 
saying that it makes sense to do so, merely that (a) it appears legal, and (b) 
it has already been implemented and deployed by a big vendor - this is not just 
a theoretical discussion.

 This specific case rejects a certificate where the two AlgorithmIdentifier 
 values in the certificate (signature and signatureAlgorithm) do not match.

Since such a “mixed” representation is legal, according to the definition of 
that specific object AlgorithmIdentifier, these two do match. Therefore, I 
think rejecting such a certificate is wrong, and a workaround is necessary. 
Such a workaround - if correctly implemented - will not detract from OpenSSL 
security, and will ensure that corner cases are handled correctly (even if we 
don’t love corner cases very much).

Uri Blumenthal

Description: S/MIME cryptographic signature
openssl-dev mailing list
To unsubscribe:

Re: [openssl-dev] [ #3665] Bug report and a patch forOpenSSL 1.0.1l (and 1.0.1k)

2015-01-19 Thread Uri Blumenthal via RT
Also, the way I read the current code (crypto/asn1/a_type.c, line 120 - it 
would (incorrectly) reject a certificate where both algorithms are encoded with 
absent parameter lists:

 if (!a || !b || a-type != b-type) return -1;

I think we all agree that such a certificate would be valid/legal?
Uri Blumenthal

openssl-dev mailing list
To unsubscribe: