390615 - Add scriptable interface to verify digital signatures

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Dave Townsend [:mossop]]

I'm working on a scriptable interface that when given a public key, some data and a signature it will verify that the data matches.

Comment 1

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This is the component mentioned. It has a very simple interface, just a single method for verifying the string of data against a signature and public key. Since it is callable from script we cannot really use binary data so the key and signature are expected to be base64 encoded.

Comment 2

[Kai Engert (:KaiE:)]

Comment on attachment 277619 [details] [diff] [review]
patch rev 1

Can you do error checking for the rv of NSSBase64_DecodeBuffer ?

Should your exit/cleanup code become too complicated, you could use the "Cleaner" functionality, for an example search for CERTCertificateCleaner.

I'm not used to the string.BeginReading() functions, but I understand they return a pointer to the beginning of the string. Do you need to call something like EndReading to clean up?


+    if (ss == SECSuccess)
+        *_retval = PR_TRUE;
+    else
+        *_retval = PR_FALSE;

Just use 
  *_retval = (ss == SECSuccess);


r=kengert with the comments addressed / verified


I would like to ask Bob for a second review, in particular because I'm not sure about the algorithm mappings.

Dave, what application is used to produce the data that you're trying to verify?

Comment 3

[Dave Townsend [:mossop]]

(In reply to comment #2)
> Dave, what application is used to produce the data that you're trying to
> verify?

The application is one that is in development and runs under XULRunner so it's basically NSS that is producing it. I don't have the code up anywhere visible right now though if needs be I can make it available but in rough:

The public key string is generated using SECKEY_EncodeDERSubjectPublicKeyInfo and then NSSBase64_EncodeItem (though I might change to an alternate base64 encoder if I can't figure out how to adjust its line breaking).

The signature string is generated using SEC_SignData and then again with NSSBase64_EncodeItem. The SECOidTag passed to SEC_SignData match those given in this patch, though that's not to say they are right of course ;)

Thanks for the quick review, I'll tidy up those issues and check the string issue.

Comment 4

[Robert Relyea]

Comment on attachment 277619 [details] [diff] [review]
patch rev 1

This patch has a lot going for it, and in general it's well thought out.

the sr- is for one simple reason: The explicit hash argument.

I think it would be better to pass in a sig AlgorithmID instead and use VFY_VerifyDataWithAlgorithm ID(). That way when new hash and signature algorithms are added, the  caller will automatically be able to use them without changes.

Other than that, the interface is fine.

Comment 5

[Dave Townsend [:mossop]]

(In reply to comment #4)
> I think it would be better to pass in a sig AlgorithmID instead and use
> VFY_VerifyDataWithAlgorithm ID(). That way when new hash and signature
> algorithms are added, the  caller will automatically be able to use them
> without changes.

Robert, I'm not really sure how that is going to be possible given that the point of the component is that it is an interface to be used by javascript. The SIGAlgorithmID type can't be handled by javascript. I guess I could create an xpcom wrapper for the type but I don't see a way of creating it without some kind of constants such as I've used for initialisation.

I guess I could just make the parameter a number that matches the values used in the SECOidTag enumeration, probably declare some of the more useful ones as constants for ease of use but you would be able to pass in anything to handle future values.

Did you have something else in mind that I've missed?

Comment 6

[Dave Townsend [:mossop]]

Requesting blocking, this bug blocks bug 378216 which is blocking-firefox3 and will want to get this checked in as soon as reviews are complete.

Comment 7

[Dave Townsend [:mossop]]

Attachment: patch rev 2

This addresses Kai's comments, uses nsPromiseFlatString rather than messing with string pointers and corrects a typo in the algorithm mapping. Need to hear back from Bob before I can proceed with anything else.

Comment 8

[Dave Townsend [:mossop]]

Attachment: patch rev 3

Ok this is different enough that I think it warrants re-review, maybe not another sr since it meets what Robert is after?

Basically changes so that the signature data passed in now contains both the signature and the hashing algorithm used. The code is fairly straightforward, just a series of steps to decode the appropriate bits out and perform the verification.

Comment 9

[Dave Townsend [:mossop]]

Attachment: unit tests

This is a unit test containing sets of data to test the component and varying tests run on that data.

Comment 10

[Robert Strong (they/them - no direct email)]

Comment on attachment 279211 [details] [diff] [review]
patch rev 3

>Index: security/manager/ssl/src/nsDataSignatureVerifier.cpp
>...
>+
>+const SEC_ASN1Template CERT_SignatureDataTemplate[] =
>+{
>+    { SEC_ASN1_SEQUENCE,
>+        0, NULL, sizeof(CERTSignedData) },
>+    { SEC_ASN1_INLINE,
>+        offsetof(CERTSignedData,signatureAlgorithm),
>+        SECOID_AlgorithmIDTemplate, },
>+    { SEC_ASN1_BIT_STRING,
>+        offsetof(CERTSignedData,signature), },
>+    { 0, }
>+};
Dave just a heads up, I had to use the SEC_ASN1_MKSUB and SEC_ASN1_SUB for this to compile on Windows.

Comment 11

[Dave Townsend [:mossop]]

Attachment: patch rev 4

This is a minor change to the previous patch. Only code to change is a check on the return value of the base 64 decoding of the key, otherwise it's a bit of macro sugar to make the template link and work correctly on windows.

Comment 12

[Dave Townsend [:mossop]]

Attachment: unit tests rev 2

I have dropped a couple of the failure cases, specifically those that contained invalid base64 data. It turns out the base64 decoder asserts when it hits bad data which would make the unit test boxes fail.

Comment 13

[Kai Engert (:KaiE:)]

Comment on attachment 279289 [details] [diff] [review]
patch rev 4

>+const SEC_ASN1Template CERT_SignatureDataTemplate[] =
>+{
>+    { SEC_ASN1_SEQUENCE,
>+        0, NULL, sizeof(CERTSignedData) },
>+    { SEC_ASN1_INLINE | SEC_ASN1_XTRN,
>+        offsetof(CERTSignedData,signatureAlgorithm),
>+        SEC_ASN1_SUB(SECOID_AlgorithmIDTemplate), },
>+    { SEC_ASN1_BIT_STRING,
>+        offsetof(CERTSignedData,signature), },
>+    { 0, }
>+};

I found a similar but slightly different template in the NSS code, could you please double check your version is correct or use the same one? When in doubt please ask Bob.

const SEC_ASN1Template CERT_SignedDataTemplate[] =
{
    { SEC_ASN1_SEQUENCE,
          0, NULL, sizeof(CERTSignedData) },
    { SEC_ASN1_ANY,
          offsetof(CERTSignedData,data), },
    { SEC_ASN1_INLINE,
          offsetof(CERTSignedData,signatureAlgorithm),
          SECOID_AlgorithmIDTemplate, },
    { SEC_ASN1_BIT_STRING,
          offsetof(CERTSignedData,signature), },
    { 0, }
};



>+    SECKEY_DestroySubjectPublicKeyInfo(pki);

Optional nit, 
  pki = nsnull;
after destroying the object, so it doesn't get accessed accidentially.


>+    // Perform the final verification
>+    signatureItem = sigData.signature;

Is there a reason for the assignment?
I guess you're simply using that as a shortcut, but when reading this code, I tried to understand why you're doing this.
I think it would be clearer to read if you used sigData.signature in the calls below.


>+    DER_ConvertBitString(&signatureItem);
>+    ss = VFY_VerifyDataWithAlgorithmID((const unsigned char*)nsPromiseFlatCString(aData).get(),
>+                                       aData.Length(), publicKey,
>+                                       &signatureItem,
>+                                       &(sigData.signatureAlgorithm),
>+                                       NULL, NULL);


r=kengert with the above things double checked.

Comment 14

[Kai Engert (:KaiE:)]

Comment on attachment 279290 [details] [diff] [review]
unit tests rev 2

r=kengert

Thanks!

Comment 15

[Dave Townsend [:mossop]]

Attachment: patch for checkin

(In reply to comment #13)
> I found a similar but slightly different template in the NSS code, could you
> please double check your version is correct or use the same one? When in doubt
> please ask Bob.

The template in NSS encapsulates the data in the signature which we don't really want. Bob gave me the template I've used here.

> Optional nit, 
>   pki = nsnull;
> after destroying the object, so it doesn't get accessed accidentially.

Done

> 
> >+    // Perform the final verification
> >+    signatureItem = sigData.signature;
> 
> Is there a reason for the assignment?

I actually copied that out of the SEC_DerSignData that I took some of the code from ;)
But yeah it reads a bit better without it so dropped that.

Final patch, carrying forward review as comments are addressed.

Comment 16

[Dave Townsend [:mossop]]

Patch and testcase checked in

Checking in public/Makefile.in;
/cvsroot/mozilla/security/manager/ssl/public/Makefile.in,v  <--  Makefile.in
new revision: 1.33; previous revision: 1.32
done
RCS file: /cvsroot/mozilla/security/manager/ssl/public/nsIDataSignatureVerifier.idl,v
done
Checking in public/nsIDataSignatureVerifier.idl;
/cvsroot/mozilla/security/manager/ssl/public/nsIDataSignatureVerifier.idl,v  <--  nsIDataSignatureVerifier.idl
initial revision: 1.1
done
Checking in src/Makefile.in;
/cvsroot/mozilla/security/manager/ssl/src/Makefile.in,v  <--  Makefile.in
new revision: 1.82; previous revision: 1.81
done
RCS file: /cvsroot/mozilla/security/manager/ssl/src/nsDataSignatureVerifier.cpp,v
done
Checking in src/nsDataSignatureVerifier.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsDataSignatureVerifier.cpp,v  <--  nsDataSignatureVerifier.cpp
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/manager/ssl/src/nsDataSignatureVerifier.h,v
done
Checking in src/nsDataSignatureVerifier.h;
/cvsroot/mozilla/security/manager/ssl/src/nsDataSignatureVerifier.h,v  <--  nsDataSignatureVerifier.h
initial revision: 1.1
done
Checking in src/nsNSSModule.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsNSSModule.cpp,v  <--  nsNSSModule.cpp
new revision: 1.42; previous revision: 1.41
done
RCS file: /cvsroot/mozilla/security/manager/ssl/tests/test_datasignatureverifier.js,v
done
Checking in tests/test_datasignatureverifier.js;
/cvsroot/mozilla/security/manager/ssl/tests/test_datasignatureverifier.js,v  <--  test_datasignatureverifier.js
initial revision: 1.1
done

Comment 17

[Dave Townsend [:mossop]]

I've filed bug 394778 since the unit tests do not appear to be running on the tinderboxes.

Comment 18

[Takanori MATSUURA]

The following build error occures when compiling libxul.so on Fedora 7 (gcc-4.1.2) x86_64. Is this pragma visibility related?

/usr/bin/ld: ../../staticlib/components/libpipnss.a(nsDataSignatureVerifier.o): 
relocation R_X86_64_PC32 against `PORT_NewArena@@NSS_3.2' can not be used when m
aking a shared object; recompile with -fPIC
/usr/bin/ld: final link failed: Bad value
collect2: ld returned 1 exit status
gmake[4]: *** [libxul.so] Error 1

Comment 19

[Dave Townsend [:mossop]]

(In reply to comment #18)
> The following build error occures when compiling libxul.so on Fedora 7
> (gcc-4.1.2) x86_64. Is this pragma visibility related?

Unfortunately I dont have an x86_64 machine handy right now to test with. Could you add FORCE_PIC = 1 to the Makefile.in there, try a build and report back on whether that works for me?

Comment 20

[Takanori MATSUURA]

(In reply to comment #19)

As far as I have checked the build log, "-fPIC" compile option is used.


I also get the following error message if "--disable-libxul" is explicitly-defined in configure script.
/usr/bin/ld: nsDataSignatureVerifier.o: relocation R_X86_64_PC32 against `PORT_N
ewArena@@NSS_3.2' can not be used when making a shared object; recompile with -f
PIC
/usr/bin/ld: final link failed: Bad value
collect2: ld returned 1 exit status
gmake[6]: *** [libpipnss.so] Error 1


When I add "ac_cv_visibility_pragma=no" to .mozconfig, no build error occurs.
We may need to add some header files to mozilla/config/system-headers.

Comment 21

[Kai Engert (:KaiE:)]

Attachment: bustage fix

I'm running into the same build failure on RHEL 5 with gcc 4.1.1.
I'm attaching a patch that fixes it for me.

Comment 22

[Kai Engert (:KaiE:)]

Comment on attachment 279601 [details] [diff] [review]
bustage fix

Requesting review for bustage fix, please feel free to check in.

This header file didn't get included before we added this patch.

Comment 23

[Dave Townsend [:mossop]]

Fix checked in

Checking in config/system-headers;
/cvsroot/mozilla/config/system-headers,v  <--  system-headers
new revision: 3.24; previous revision: 3.23
done

Comment 24

[Robert Relyea]

Comment on attachment 279477 [details] [diff] [review]
patch for checkin

sr+ relyea

David's template is correct for detached signatures, which is what he wants.

The other template encapsulated the data with the signature.

bob

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

This seems like a very specialized interface.  
It takes public keys (not certs) that are base64 encoded. 
Such things are not commonly found in the wild.  
I suspect this interface will have only one user in its lifetime.
I'd prefer that the interface be more widely usable.
I'm surprised that no one else made this observation before now.

Comment 26

[Kai Engert (:KaiE:)]

See also: bug 685852