679377 - RFE: be more forgiving of malformed(?) kerberos CMS SignedData messages

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Elio Maldonado]

Reported by Nalin Dahyabhai:

Description of problem:
When parsing PKINIT preauthentication responses from a KDC running WS2003, my
code is failing to verify the signature correctly.  On examination of the
signed data inside of the enveloped data, it appears that the digest algorithm
given for the signed data is that of a signature algorithm (in my case
SEC_OID_PKCS1_SHA1_WITH_RSA_ENCRYPTION) rather than a digest algorithm (I'd
expect SEC_OID_SHA1).

I think the party generating the message is doing it wrong, but I'd like to
parse the message successfully anyway.

Version-Release number of selected component (if applicable):
nss-3.12.10-6.fc16.x86_64

How reproducible:
Always

Steps to Reproduce:
I'll attach the data that I have.

Actual results:
Error 18 while attempting to verify the data.

Expected results:
No error.

Comment 1

[Elio Maldonado]

Attachment: Possible patch -- there is possibly a better way

This is Nalin's patch made in nss-3.12.10

Comment 2

[Elio Maldonado]

Attachment: test program

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

This sounds like another case of 
  "NSS doesn't accept malformed data from program X, so change NSS, not X".
Beware the "bug compatibility" black hole.

Comment 4

[Robert Relyea]

Unforutnately 'X' is Microsoft Server/2008 ;(.

bob

Comment 5

[Robert Relyea]

Attachment: Map hash oids back to real hash oids if someone sends up signature algorithms.

Comment 6

[Elio Maldonado]

Comment on attachment 556074 [details] [diff] [review]
Map hash oids back to real hash oids if someone sends up signature algorithms.

>? patch
>Index: cmslocal.h
> 
>+/*
>+ * local function to handle compatibility issues
Add
   * maps the signature algorithm oid tag to a digest one if needed.
>+ */
>+SECOidTag NSS_CMSUtil_MapSignAlgs(SECOidTag signAlg);



>Index: cmssigdata.c
>===================================================================
Fine as it is.

>Index: cmssiginfo.c

While studying the code I added some comments for my own understanding
which I think other maintainers of the code may appreciate.

>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/smime/cmssiginfo.c,v

> SECOidData *
> NSS_CMSSignerInfo_GetDigestAlg(NSSCMSSignerInfo *signerinfo)
> {
>-    return SECOID_FindOID (&(signerinfo->digestAlg.algorithm));
>+    SECOidData *algdata;
>+    SECOidTag   algtag;
>+
>+    algdata = SECOID_FindOID (&(signerinfo->digestAlg.algorithm));
Comment
	/* unrecognized gorithm, the NULL will force an error */

>+    if (algdata == NULL) {
>+	return algdata;
>+    }

This comment helped me.

    /* We may have been given signer instead of a digest algorithm oid
     * data. This call will map to digest otherwise it leaves it alone
     * as it's true digest tag data and we can find the oid from tag
     */

>+    algtag = NSS_CMSUtil_MapSignAlgs(algdata->offset);
>+    if (algtag != algdata->offset) {

and this one
	/* mapping succeeded we have a digest algorithm tag */

>+	algdata = SECOID_FindOIDByTag(algtag);
>+    }
>+
and here
     /* Here the algorithm tag is for a digest algorithm  */

>+    return algdata;
>+
> }
> 

>Index: cmsutil.c
>===================================================================

>+/*
>+ * Map a sign algorthim to a digest algorithm.

Fix typo: s/algorthim/aglorithm/

>+ * This is used to handle incorrectly formatted packages sent to us
>+ * from Windows 2003.
>+ */

Only Windows 2003? 

>+SECOidTag
>+NSS_CMSUtil_MapSignAlgs(SECOidTag signAlg)
>+{
>+    switch (signAlg) {
>+    case SEC_OID_PKCS1_MD2_WITH_RSA_ENCRYPTION:
>+	return SEC_OID_MD2;
>+	break;
>+    case SEC_OID_PKCS1_MD5_WITH_RSA_ENCRYPTION:
>+	return SEC_OID_MD5;
>+	break;
>+    case SEC_OID_PKCS1_SHA1_WITH_RSA_ENCRYPTION:
>+    case SEC_OID_ANSIX962_ECDSA_SHA1_SIGNATURE:
>+    case SEC_OID_ANSIX9_DSA_SIGNATURE_WITH_SHA1_DIGEST:
>+	return SEC_OID_SHA1;
>+	break;
>+    case SEC_OID_PKCS1_SHA256_WITH_RSA_ENCRYPTION:
>+    case SEC_OID_ANSIX962_ECDSA_SHA256_SIGNATURE:
>+	return SEC_OID_SHA256;
>+	break;
>+    case SEC_OID_PKCS1_SHA384_WITH_RSA_ENCRYPTION:
>+    case SEC_OID_ANSIX962_ECDSA_SHA384_SIGNATURE:
>+	return SEC_OID_SHA384;
>+	break;
>+    case SEC_OID_PKCS1_SHA512_WITH_RSA_ENCRYPTION:
>+    case SEC_OID_ANSIX962_ECDSA_SHA512_SIGNATURE:
>+	return SEC_OID_SHA512;
>+	break;
>+    default:
>+	break;
>+    }
>+    /* not one of the algtags incorrectly sent to us*/

I would expand the comment to

    /* not one of the algorithm tags incorrectly sent to us,
     * but a true digest algorithm tag so leave it as it is.
     */
>+    return signAlg;
>+}
>+

Comment 7

[Elio Maldonado]

Comment on attachment 556074 [details] [diff] [review]
Map hash oids back to real hash oids if someone sends up signature algorithms.

r+, Please incorporate some of those comments or a variant of them.

Comment 8

[Wan-Teh Chang]

Nalin: does a KDC running Windows Server 2008 generate
correct PKINIT preauthentication responses?  We need the
exact versions of Windows Server that need this workaround,
so we know when this workaround can be removed.

There's always the risk that new code will start to
depend on the workaround.  Adding an option to enable
the workaround only for parsing PKINIT preauthentication
responses would be a good idea.

We should submit a bug report to Microsoft if Windows Server
2008 still has this bug.

Comment 9

[Nalin Dahyabhai]

(In reply to Wan-Teh Chang from comment #8)
> Nalin: does a KDC running Windows Server 2008 generate
> correct PKINIT preauthentication responses?  We need the
> exact versions of Windows Server that need this workaround,
> so we know when this workaround can be removed.

No, I haven't seen Server 2008 or 2008r2 do this.  I only encountered it with 2003 or m2003r2, though I was unable to test with any older server versions.

Comment 10

[Robert Relyea]

Checking in cmsutil.c;
/cvsroot/mozilla/security/nss/lib/smime/cmsutil.c,v  <--  cmsutil.c
new revision: 1.17; previous revision: 1.16
done
Checking in cmssiginfo.c;
/cvsroot/mozilla/security/nss/lib/smime/cmssiginfo.c,v  <--  cmssiginfo.c
new revision: 1.36; previous revision: 1.35
done
Checking in cmssigdata.c;
/cvsroot/mozilla/security/nss/lib/smime/cmssigdata.c,v  <--  cmssigdata.c
new revision: 1.32; previous revision: 1.31
done
Checking in cmslocal.h;
/cvsroot/mozilla/security/nss/lib/smime/cmslocal.h,v  <--  cmslocal.h
new revision: 1.8; previous revision: 1.7
done