Wrong enveloped signatures generation about xml-signer HOT 31 CLOSED

Thanks. I'll take a look as soon as I can.

from xml-signer.

It seems you have the Xml to be signed in a DOM document. What happens if you save the document first and then sign it?

A potential problem when using a DOM document directly that has not been saved is that there is no base URI for the document. It may be the lack of a URI means the references are not being named correctly.

Can you provide an example of a source XML document and the signed document that shows this error? It doesn't have to be an Xml document with real data.

from xml-signer.

When I suggest saving the source Xml as a document, of course I also mean that you change the input resource in your script to reference the saved document.

from xml-signer.

I have tested with distinct resource types (string, xmlDocument and file) with same result.

Here goes my test with file type resource:
test-documents.zip

code:

    openssl_pkcs12_read(
                file_get_contents($pfxFilePath),
                $certData,
                $password
            );

            $filenam = tempnam(sys_get_temp_dir(), 'test-document-');
            file_put_contents($filenam, $this->dom()->saveXML());
            $this->signedXml = XAdES::signDocument(
                new InputResourceInfo(
                    $filenam, // The source document
                    ResourceInfo::file, // The source is a url
                    dirname($signedFilePath), // The location to save the signed document
                    basename($signedFilePath), //$storeFilename, // The name of the file to save the signed document in,
                    null,
                    false
                ),
                new CertificateResourceInfo($certData['cert'], ResourceInfo::string | ResourceInfo::pem),
                new KeyResourceInfo($certData['pkey'], ResourceInfo::string | ResourceInfo::pem),
            );
            $this->signedXmlPath = $signedFilePath;
            XAdES::verifyDocument(
                $signedFilePath
            );

from xml-signer.

I've been taking a look at what makes the digest value to differ between original and the signed document and it seem that there is an issue on how namespaces are handled on SignedProperties digest:

The namespaces used while signing are:

<xa:SignedProperties xmlns:ds="http://www.w3.org/2000/09/xmldsig#" xmlns:xa="http://uri.etsi.org/01903/v1.3.2#" Id="signed-properties">...</xa:SignedProperties>

While verification (and ETSICC) uses these namespaces:

<xa:SignedProperties xmlns:T="urn:ticketbai:emision" xmlns:ds="http://www.w3.org/2000/09/xmldsig#" xmlns:xa="http://uri.etsi.org/01903/v1.3.2#" Id="signed-properties">...</xa:SignedProperties>

I've been struggling with how the code handles this but I'm a bit lost on how it is working :(

from xml-signer.

I can reproduce this issue by adding a prefix/namespace to the root element of any document.

The validation fails because, as you note, the prefix/namespace from the root is missing from the signed properties element so the reference hash generated during signing is not correct. During validation by the Xml Signer the prefix/namespace from the root is added to the signed properties element so the reference hash created is correct (in the sense that it is the same one generated by ETSICC) but is different to the one recorded in the signature so the validation fails.

So why is the the prefix/namespace from the root is missing from the signed properties element when the signature is created?

I believe this is because the signed properties element is new. At the moment, when the hash of the new element is created it is not yet part of the original document so the reference hash generation process does not know about any prefixes and namespaces attached to the root element of the original document. As a result, when the C14N function is called to produce a canonicalized XML string the additional namespace is missing so the generated hash value is wrong.

By contrast, when the same canonicalization function is used during validation, the signed properties element already exists so the canonicalization result includes any prefixes/namespaces from the root element.

This weekend I'll try to address this issue.

from xml-signer.

I made a pull request #10 based on your comment about having the signed properties inside the original document to get the namespaces injected when the hash is generated.

Hope this fixes the issue or helps you getting a solution for it.

from xml-signer.

Thanks, I'll take a look. Yours may the only workable approach. I've been trying other different ways to add a valid attached signature but without success. There are a couple of reasons I chose not to fill the base class with input XML so I'll check if either reason is still valid:

1. When a detached signature is requested it has to be generated without any of the input XML. Instead the signature must include a reference to the source file. All my use of XAdES requires detached signatures. This focus may be why I missed the case of adding an attached signature to a document with a namespace on the root element.

2. At least in the early phase of the development of the library, adding the signature directly to the input XML document resulted in libxml (the XML library wrapped by PHP to provide the DOMDocument class) adding namespaces with the prefix 'default' for namespaces on the root.

Hopefully both of these case no longer apply.

from xml-signer.

I found a bug on my changes when the InputResourceInfo was xmlDocument type. DOM had to be cloned on the generateDomDocument method (L126).

About your second point, I have tested removing the namespace to my document and doesn't seem to generate any "default" namespace, everything seems to work correctly. When document is of DOM type cloning it also avoids any unexpected pollution of the original document.

I also tested my code against detached "file" type InputResourceInfo and the DSS Demonstration WebApp returns that everything is OK. I haven't tested against URL type thought.

BTW, after your yesterday's commit my pull request has conflicts, tell me if I can do something to resolve them.

from xml-signer.

Thanks for letting me know and for the fix. I looked at your changes last night and have incorporated them into my copy but in a slightly different way - in part to avoid the change to line-ends. I'll find time to test today but it's great to know you have a solution that works for you at the moment.

The reason for my preference for a slightly different implementation is the XMLSecurityDSig class which is taken from Rob Richards XML DSig implementation. This class doesn't know anything about XAdES so I've tried to limit changes to this file to those which fix XMLDSig problems or extend it to support other XML DSig specifications (such as XPath filters). Also, by adding your change to load the original document in the constructor of this class it will also be called on all invocations of the descendent XAdES class not just for new signature creation. For example, calls to the validation functions or adding a new timestamp or a counter-signature because in these cases the input document will already have a signature element so the problem with generating the correct canonicalized XML string should not arise.

So in my copy I've moved your change to the constructor into the signDocument function.

A problem now is that because I'm not just taking your PR you don't show up as a contributor and I want to find a way to make that happen because you have identified and found a solution to a real issue for which I'm really grateful.

from xml-signer.

I have no problem at not appearing as a contributor. Don't worry about that and thanks for working on this.

from xml-signer.

I've pushed updates that incorporate the changes you have suggested. If you review the changes you will see where they have been made, The location of some changes are different but you will recognise them.

Some testing has been done but more is required. I have had success with attached and detached signatures when applied to files and a URL source. However, I'm hoping you will be able to find enough time to try the changes to see if they work for you. My impression is that you are signing XML in a DOMDocument rather than a file. If so, it will be great if you are able to try the changes with an example document.

Thanks again.

from xml-signer.

I'll try this and give any feedback to you ASAP, thanks to you.

from xml-signer.

Hi @bseddon,

I have tested against string, xmlDocument and file types and all of them return positive answers.
So I can confirm that this is working with your changes. ✔️

Buuut (I'm a pain in the a**), I may be having another kind of trouble (this already happened before too, is not really a new issue) and don't know if I should create a new Issue for that...

When I verify the signed document I keep getting the next exception:
The certificate issuer in the signature does not match the certificate issuer number

The issue seems to come from the IssuerSerialV2 verification. This element is optional so I have removed it from the code making SigningCertificateV2 L105 falsy, and everything seems to validate correctly after that.

I asked for some help on this and they told me that decoded ASN1 is:

SEQUENCE (2 elem) 

Offset: 0 

Length: 2+76 

(constructed) 

Value: 

(2 elem) 

  SEQUENCE (3 elem) 

    SET (1 elem) 

      SEQUENCE (2 elem) 

        OBJECT IDENTIFIER 2.5.4.6 countryName (X.520 DN component) 

        PrintableString ES 

    SET (1 elem) 

      SEQUENCE (2 elem) 

        OBJECT IDENTIFIER 2.5.4.10 organizationName (X.520 DN component) 

        UTF8String IZENPE S.A. 

    SET (1 elem) 

      SEQUENCE (2 elem) 

        OBJECT IDENTIFIER 2.5.4.3 commonName (X.520 DN component) 

        UTF8String Izenpe.com 

  INTEGER (126 bit) 44226866073358392262219520495746478787 

which apparently is not a correct IssuerSerial, which should be:

IssuerSerial  ::=  SEQUENCE { 

       issuer         GeneralNames, 

       serial         CertificateSerialNumber, 

       issuerUID      UniqueIdentifier OPTIONAL 

   } 

I don't really understand anything about that, but that's what they told me 😅

This is beyond my knowledge on certificate insides, so I was thinking on making IssuerSerialV2 really optional somehow... but if you know what they are talking about you may fix it :)

from xml-signer.

Thanks for letting me know about your test results. So let's move on to serial numbers...

To be compliant with the XAdES 2016 specification you have to use the V2 structure. If you are working in a closed environment where you can dictate the specific classes that will be used in a signature then your solution could work. However, if your signatures are going to be read by 3rd parties you probably need to use V2. For example when using the ETSICC you will get away with using the V1 structures if you use the first option under the 'upload' button. However if you use the third option, which requires conformance with the 2016 profiles, then V2 is mandatory.

From what you write, I don't think the issue is with the serial number but before getting to that, we need to be sure of the serial number being returned by the PHP code. If there is an issue reading the certificate that issue needs to be addressed. The thing you need to be sure about is the serial number. My work environment is Windows and on Windows there is the certificate manager (see the screenshot). So I would right click on a .crt (containing a certificate in a PEM format) or on .p12 file (containing a certificate in a PKCS 12 format) and select the 'open' option to display the certificate details. Select the 'Details' tab where you will see the number in HEX format.

Alternatively, if OpenSSL is installed you are able to dump the certificate contents using one of its command parameter sequences.

Either way, the serial number used by the PHP should be the same one shown by Windows or OpenSSL.

However... what that message is saying is not a problem with the serial number. Instead, that the issuer details recorded in the certificate you are using does not appear to be subject details recorded in the issuer's certificate.

That is, the 'issuer' of your certificate is the 'subject' of their own certificate. You are the subject of your own certificate. The two should match but this test is failing.

Are you able to share your certificate with me so I can take a look? I'm not interested in the private key just the public certificate.

from xml-signer.

Here is a screenshot of a test certificate generated using a certificate I use as a certificate authority root. I've shown the content of the 'issuer' and 'subject' fields side-by-side.

My issuing certificate has as its 'subject' details the same content as the 'issuer' details shown in the screenshot (that is, of the issued certificate).

from xml-signer.

To be compliant with the XAdES 2016 specification you have to use the V2 structure. If you are working in a closed environment where you can dictate the specific classes that will be used in a signature then your solution could work. However, if your signatures are going to be read by 3rd parties you probably need to use V2. For example when using the ETSICC you will get away with using the V1 structures if you use the first option under the 'upload' button. However if you use the third option, which requires conformance with the 2016 profiles, then V2 is mandatory.

I understand. I didn't mean to use V1 or avoid SigningCertificateV2, I meant that IssuerSerialV2 inside SigningCertificateV2 is optional if you look at specification's xsd:

<xsd:element name="IssuerSerialV2" type="xsd:base64Binary" minOccurs="0"/>

So in my case removing it just serves the purpose

Are you able to share your certificate with me so I can take a look? I'm not interested in the private key just the public certificate.

No problem, this is a public certificate for development purpose so I could give you the private key too if you want.
enpresaZigilua.crt.gz

I have also tested this with my own private Class 2 CA User Certificate issued by Spanish FNMT-RCM, and same Exception is thrown on verification.

from xml-signer.

Fair enough, yes it is optional. I'll add an option to the signature functions that allows a caller to choose not include the issuer serial number. And thanks for the certificate.

from xml-signer.

When I use your certificate to create a signature (before it gets to the sign function where it would need a private key) I see there is a warning that the issuer certificate cannot be retrieved. The code below reproduces the essential problem with the code before today that I see. The issue is that the Alternative Information Information (AIA) section of the certificate's extensions reports that the issuer certificate should be retrieved from:

http://www.izenpe.com/contenidos/informacion/cas_izenpe/es_cas/adjuntos/CCEER_cert_sha256.crt

The code uses the function file_get_contents to retrieve the file but fails, I think, because the server redirects to https. file_get_contents can cope with the redirect but can't access the file over https because (at least in my case) it doesn't have access to a certificate bundle which is needed to verify the peer connection. As a result, PHP reports an SSL warning and the function returns false. At this point the issuer certificate is not retrieved so the issuerSerialV2 class content is invalid. In this case, the class should not be created because, as you point out, the class is optional.

The immediate fix is to add a stream context to the call to file_get_contents to tell it not verify the peer. The code snipets below illustrate the difference between using a stream context and not.

I see an error in this case:

$result = file_get_contents( $urlOfIssuerCertificate );

Adding the stream context resolves the issue:

$result = file_get_contents( $urlOfIssuerCertificate, false, stream_context_create( array("ssl"=>array("verify_peer"=>false, "verify_peer_name"=>false ) ) ) );

This change has been made in Ocsp.php in the requestor library which the xml-signer code relies on.

from xml-signer.

However, I don't think this is the whole story. The error you reported in your earlier post occurs because the issuer details in the certificate you attached and the subject details in the issuer certificate are not the same. Below I have reproduced the respective DN strings:

The issuer details reported in the end certificate
CN=CA de Ciudadanos y Entidades (4) - DESARROLLO, OU=NZZ Ziurtagiri publikoa - Certificado publico SCI, O=IZENPE S.A., C=ES

The subject details reported in the issuer certificate (these should be the same)
CN=Herritar eta Erakundeen CA - CA de Ciudadanos y Entidades (4), OU=NZZ Ziurtagiri publikoa - Certificado publico SCI, O=IZENPE S.A., C=ES

As you can see, the common name (CN) value is not the same. Of course a human can see they are the same as the first is contained in the CN of the second. However, the test only checks values and, as a result, the test to compare the two issuer details on XAdES.php line 1162 or 1192 will fail with the message you included in your earlier post.

I guess what's needed is to include an option that controls whether the issuer details are compared.

from xml-signer.

The V2 serial number is already conditional. If you look in ./src/xml/SigningCertificateV2.php on line 184 you will see that the serial number is only added to the signing certificate instance if the is has been possible to retrieve the issuer certificate, That is, $issuerSerialDER is null unless the issuer certificate can be retrieved.

from xml-signer.

OK, I understand that the issuer/subject data on my certificate do not match, so I have to avoid the the issuer certificate to be retrieved from it.

As I have no idea on how to do that 😅 I just overloaded the createQualifyingProperties method to call my own SigningCertificateV2::fromCertificate where I ignore the issuer certificate to make this work.

So this issue has a valid workaround for me, where there is no need to modify you original library :)

Anyway I will investigate a bit more about this, as both of the certificates I use are failing on the same point. I can understand the development certificate may have an error, but it seems a bit strange on the other case, will tell you more when I know.

Thanks for all your help!

from xml-signer.

Hello there,

I've been testing this against a "Private individual" certificate issued by the spanish FNMT-RCM (official agency that issues certificates in Spain).

The issuer of my certificate in this case is:
C = ES, O = FNMT-RCM, OU = Ceres, CN = AC FNMT Usuarios

But if I verify the generated signed document, I can see that verification process fails when verifying the certificate issuer with 'The certificate issuer in the signature does not match the certificate issuer number' error.

I took a look at what comparison is made and can see this two different issuers are compared:

• $dnNames: "OU=AC RAIZ FNMT-RCM, O=FNMT-RCM, C=ES"
• $issuer: "CN=AC FNMT Usuarios, OU=Ceres, O=FNMT-RCM, C=ES"

The first one matches the issuing Root CA Certificate's Subject and the second one is the certificate used to issue "Private individual" certificates. You can see this hierarchy here: (https://www.sede.fnmt.gob.es/en/descargas/certificados-raiz-de-la-fnmt), where:

• AC Raíz FNMT-RCM = Root CA
• Certificados subordinados = Subordinate certificates

I would say that there is a bug somewhere, but don't know which certificates should really be compared.

from xml-signer.

I think we have different requirements. The issuer shown in the certificates I will use will always be the same as the subject of the issuer's certificate. As a result, the code is written to include this test. In my scenario it is an error for these DNs to be different.

As you wrote yesterday, you have a rational workaround which is to ensure the issuer is not included within the SigningCertificateV2 class. Because the issuer is not included, the test to compare the DNs does not take place.

I think your workaround is a great one. This and your solution to the original problem show you really understand what's going on.

from xml-signer.

Yes my workaround works perfectly for me, no problem from that point of view. (In fact I'm OK with closing this issue)

I was trying to guess why the verification didn't work with my certificates, and checking if there was any bug with this. Just trying to help here...

I will make just one clarification to make sure I explained myself on the last comment. In this case the certificate issuer and subject are identical, but there is a root CA involved that is being also compared, so even when the certificate issuer and the issuer's subject are identical the verification fails:

ROOT CA certificate -> Subordinate certificate -> My certificate

• My certificate:
  Subject: C = ES, serialNumber = XXXXXXXXXXXX, CN=MY-PERSONAL-DATA
  Issuer: C = ES, O = FNMT-RCM, OU = Ceres, CN = AC FNMT Usuarios

• Subordinate certificate:
  Subject: C = ES, O = FNMT-RCM, OU = Ceres, CN = AC FNMT Usuarios
  Issuer: C = ES, O = FNMT-RCM, OU = AC RAIZ FNMT-RCM

• Root CA Certificate:
  Subject: C = ES, O = FNMT-RCM, OU = AC RAIZ FNMT-RCM
  Issuer: C = ES, O = FNMT-RCM, OU = AC RAIZ FNMT-RCM

If that verification works for you and you think that it doesn't matter that it fails in this case, I'm OK with it :)

Thanks again for all you help and for this life saving library.

from xml-signer.

Thanks. As you might guess, I don't see an error that is not expected so I can't reproduce an issue to fix. But it may be we are referring to different things. I do see an error using the certificate you posted but that error is is expected as explained above.

The error with the message you see is generated if the test on line 1162 or 1192 of XAdES.php returns false.

Below I've created two scripts which work for me and that represent the essence of the tests on line 1162 and 1192. Note that neither scripts import anything so it will be necessary to add relevant import statements for them to run. It will be great if you are able to run the test to see what results you see. Using the scripts avoids an error being reported for some other reason. They also make clear what I think should be the cause of the error you see. I'm hoping that if my understanding of the root cause is wrong you will be able to see that I've thinking about the wrong problem from the issue I am trying to isolate.

You can see the first script uses values from your last message. For me, this test returns true.

// From your message...
// Issuer: C = ES, O = FNMT-RCM, OU = Ceres, CN = AC FNMT Usuarios
$issuerNames = "C = ES, O = FNMT-RCM, OU = Ceres, CN = AC FNMT Usuarios";
// Subordinte Subject: C = ES, O = FNMT-RCM, OU = Ceres, CN = AC FNMT Usuarios
$subjectNames = "C = ES, O = FNMT-RCM, OU = Ceres, CN = AC FNMT Usuarios";

$result = CertificateInfo::compareIssuerStrings( $issuerNames, $subjectNames );

A more complete example is shown in the following code block. This just about reproduces the whole test while ignoring the serial number. The example references the certificate you uploaded which results in a negative test for me, again for the reason described in an earlier post. However, if I use one of my own certificates it returns a positive result so in principle the code appears to work.

In the example, it:

1. loads the certificate you uploaded and which I saved into the folder from which the script is run;
2. calls the static function Ocsp::getCertificate to return relevant details about the certificate and, if it's accessible, it returns the issuer certificate;
3. accesses the issuer names from the original certificate;
4. the code assumes the issuer certificate will be retrieved so it accesses the issuer names; and
5. the respective names are converted to a string and compared.

$loader = new CertificateLoader();
// This path assumes the certificate file is in the same directory as the file containing the script
$certificate = $loader->fromFile( __DIR__ . '/enpresaZigilua.crt' );

// Get the issuer certificate
list( $certificate, $certificateInfo, $ocspResponderUrl, $issuerCertBytes, $issuerCertificate ) = array_values( Ocsp::getCertificate( $certificate, null ) );
/** @var \lyquidity\Asn1\Element\Sequence $certificate */
/** @var CertificateInfo $certificateInfo */
/** @var \lyquidity\Asn1\Element\Sequence $issuerCertificate */

// Get the list of names...
$info = new CertificateInfo();
// ...first the issuer of the end certificate...
$issuerNames = $info->extractIssuer( $certificate );
// ...then the subject of the issuer certificate
$subjectNames = $info->extractSubject( $issuerCertificate );

// The two names should be the same
$issuerDNString = $info->getDNStringFromNames( $issuerNames );
$subjectDNString = $info->getDNStringFromNames( $subjectNames );

$result = CertificateInfo::compareIssuerStrings( $issuerDNString, $subjectDNString );

from xml-signer.

I do see an error using the certificate you posted but that error is is expected as explained above

Yes, that error is expected and I understood why is happening. Is not the same issue I'm referring now.

Both scripts you provide work without any error with this certificate too. These two lines return "C = ES, O = FNMT-RCM, OU = Ceres, CN = AC FNMT Usuarios".

$issuerDNString = $info->getDNStringFromNames( $issuerNames );
$subjectDNString = $info->getDNStringFromNames( $subjectNames );

And compareIssuerStrings returns true as expected

The problem happens only when the certificate is retrieved from the IssuerSerialV2 field on the signed document. In that case $dnNames returns the "Root CA" certificates subject instead of the intermediate certificates subject. I'm not sure if the error (if any) is on the verification or the IssuerSerialV2 generation process neither.

Do you have any certificate where a chain of certificates is used to issue the last certificate? I could share my public CRT with you, but you wouldn't be able to sign a document with it...

from xml-signer.

It will be great if you can share the certificate. The V2 class generation occurs before the document is signed. As you mention, I'll be able to see what's being included in the V2 class when there are is an intermediate and then root certificate in the chain.

It will also be great if you are able to send a signed document. Any recipient should be able to verify the document and, so, hopefully I will see the verification problem you experience.

from xml-signer.

The message is deleted. I think you are correct that the issue occurs when there is one or more intermediate certificates in the chain. The issue is likely to be on line 153 of ./xml-signer/src/xml/SigningCertificateV2.php. At this point the code has retrieved the issuer certificate so its serial number can be added to the IssuerSerialV2 class.

The code on line 153 adds some DNs to the IssuerSerialV2 class as well. These should be the names of the certificate subject but as you will see, the names of the issuer are added. That is, the issuer of the issuer. When the issuer is the trust anchor (CA root) then the issuer and subject are the same. Because my test certificate's issuer is also a trust anchor the error you see does not occur for me (because the issuer's issuer is also the subject).

It will be great if you are able to change the line (153) which should be:

$certificateInfo->extractIssuer( $issuerCertificate )

to

$certificateInfo->extractSubject( $issuerCertificate )

This change has been pushed to the repository.

I believe this change should fix the error you see. If this does not fix it, please send me a copy of a new signed document you create (and which still does not verify).

Thank you for your gentle persistence, you got me there in the end. With your help three issue have now (or are in the process of being) addressed which is great.

from xml-signer.

from xml-signer.

Thanks for checking

from xml-signer.