EVerest / cbexigen

cbExiGen - The V2GTP EXI codec generator for cbV2G
Apache License 2.0
29 stars 16 forks source link

Datatype of X509SerialNumber #54

Open Thoren-G opened 9 months ago

Thoren-G commented 9 months ago

Hello again,

I have discovered a problem related to the X509SerialNumber inside the CertificateInstallationReq and CertificateUpdateReq.

The X509SerialNumber can be up to 20 octets by definition (RFC5280 4.1.2.2).

The XSD defines it as an xs:integer type, which has no maximum.

However, cbEXIgen handles xs:integer as int32. This will cause the decoder to fail for long serial numbers.

// Element: definition=complex; name={http://www.w3.org/2000/09/xmldsig#}X509IssuerSerial; type={http://www.w3.org/2000/09/xmldsig#}X509IssuerSerialType; base type=; content type=ELEMENT-ONLY;
//          abstract=False; final=False;
// Particle: X509IssuerName, string (1, 1); X509SerialNumber, integer (1, 1);
struct iso2_X509IssuerSerialType {
    // X509IssuerName, string
    struct {
        char characters[iso2_X509IssuerName_CHARACTER_SIZE];
        uint16_t charactersLen;
    } X509IssuerName;

    // X509SerialNumber, integer (base: decimal)
    int32_t X509SerialNumber;
};

This problem seems to effect all standards, but for DIN the corresponding encode and decode functions do exist but are never used. So this is actually only a problem for ISO-2 and ISO-20.

Best regards

barsnick commented 9 months ago

Hi @Thoren-G, we acknowledge this problem. We do not have a proper xs:integer coder yet.

Too bad that exactly one single occurrence of such a special type made its made into the standard. ;-)

SiebrenW commented 8 months ago

OpenV2G had a recent update regarding this in 2022 in the iso1X509IssuerSerialType struct, their solution was an array of 20 bytes;

    struct {
        /** a sign value */
        int negative;
        /* container size */
        /* size_t size; iso1X509IssuerSerialType_X509SerialNumber_BYTES_SIZE */
        /** array data container */
        /* For negative values, the Unsigned Integer holds the
         * magnitude of the value minus 1 */
        uint8_t data[iso1X509IssuerSerialType_X509SerialNumber_BYTES_SIZE];
        /** array length (len <= size) */
        size_t len;
    } X509SerialNumber;

We ought to look at this implementation. I was wondering why they didn't define a struct called bigInt or something, but I suppose it's hardly necessary for a single occerence.

SiebrenW commented 7 months ago

I think I resolved this in #65 Please have a look.

kwoyhope commented 6 months ago

IMHO, "[RFC5280] 4.1.2.2. Serial Number"(below) might have a typo.

The serial number MUST be a positive integer assigned by the CA to each certificate. It MUST be unique for each certificate issued by a given CA (i.e., the issuer name and serial number identify a unique certificate). CAs MUST force the serialNumber to be a non-negative integer. Given the uniqueness requirements above, serial numbers can be expected to contain long integers. Certificate users MUST be able to handle serialNumber values up to 20 octets. Conforming CAs MUST NOT use serialNumber values longer than 20 octets. Note: Non-conforming CAs may issue certificates with serial numbers that are negative or zero. Certificate users SHOULD be prepared to gracefully handle such certificates.

It seems that the "20 octests" might be understood more correctly with the meaning of "20 digits". ie, 'Serial Number' is not a byte stream but single value of max 99999999999999999999. So, it might be enough(more safe) just changing 'X509SerialNumber' type from 'int32_t' to 'int64_t'. And furthermore, using struct {byte[20]} type for 'X509SerialNumber' like OpenV2G, may make an interoperability(EXI codec) issue with others(ex, SwithEV RISE-V2G).

SiebrenW commented 6 months ago

I very much doubt that's an error nor that they mean something other than the integer length in bytes. Octet is another term for byte, they keep calling it an integer and never a string. Everything is indicating an integer of 20 bytes, not a string.

And considering that openv2g updated this specific datatype to fill 20 bytes should also be enough reason to follow their example; EVs may use this.

I also found this insight on SO saying 16 bytes may be enough, which is still twice as much as a u64.

https://stackoverflow.com/questions/37031043/minimum-and-maximum-length-of-x509-serialnumber

So especially since this concerns security and data from outside sources, and following from other examples, we ought to stick on the safe side.

kwoyhope commented 6 months ago

I understand your concern, but I didn't mean 'octect' is string. (ie, it's just a big NUMBER, not likely UUID) (1) What I suspect is, "Certificate users MUST be able to handle serialNumber values up to 20 octets."([RFC5280] 4.1.2.2. Serial Number) is more natural and reasonable when it is changed to "Certificate users MUST be able to handle serialNumber values up to 20 digits." ie, number of '99999999999999999999' -> uint64 could cover it, what I meant is not "99999999999999999999" char strings.

(2) in "[RFC2459] 4.1.2.2 Serial number"

The serial number is an integer assigned by the CA to each certificate. It MUST be unique for each certificate issued by a given CA (i.e., the issuer name and serial number identify a unique certificate).

in my understanding, ASN.1 also have an ambiguity of interpreting INTEGER(https://www.oss.com/asn1/knowledge-center/asn1-java/asn1java-integer-type-limitations.html) type. (Frankly speaking, I think ASN.1 and EXI are not good for used in embedded communication protocol specification in principle.)

(3) I can't tell EV(ex, Vector Infomatik) may use different appoach, but the latest OpenV2G generate different EXI stream(near end of stream, see below log) with SwitchEV(as I know SwitchEV already have coworked with the Hubject). I agree that OpenV2G is used in many way, but I believe that none of major EV OEMs use(adopt in vehicle) it. (some minor OEM might use it(Open Source), but they probably may not support Plug&Charge).

IMHO, OpenV2G v0.9.4(30 January 2018) is the stable release(inludes fixing eMAID fragment issue), and I couldn't find where it(supporting big integer) from. (no related issue ticket found) image

[FragmentEncoding] {"CertificateInstallationReq": {"Id": "id1", "OEMProvisioningCert": "MIIB4DCCAYagAwIBAgICMEAwCgYIKoZIzj0EAwIwRzESMBAGA1UEAwwJT0VNU3ViQ0EyMQ8wDQYDVQQKDAZTd2l0Y2gxCzAJBgNVBAYTAlVLMRMwEQYKCZImiZPyLGQBGRYDT0VNMB4XDTI0MDIyNjA0NTY1M1oXDTI4MDIyNTA0NTY1M1owSTEUMBIGA1UEAwwLT0VNUHJvdkNlcnQxDzANBgNVBAoMBlN3aXRjaDELMAkGA1UEBhMCVUsxEzARBgoJkiaJk/IsZAEZFgNPRU0wWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAARrmpG5Hh5iy8d3d/so7MTo0wkfPl1aBGveN2+N0haZgONh8oiNjx/QgHuBy9/iHgUvGloEgZDp91uNAYdnT6ELo2AwXjAMBgNVHRMBAf8EAjAAMA4GA1UdDwEB/wQEAwIDiDAdBgNVHQ4EFgQUnf5+WbX55/XlbzrXIET2QnCYDfwwHwYDVR0jBBgwFoAUz4bmolpD03UsMkh3NNOpty786dQwCgYIKoZIzj0EAwIDSAAwRQIhAJKzVqWHhAaTcfS+JW7uH63mmMIXFxmq8/UGMwGD6bC6AiArsTQGlynLXUMeUznVtXsLzKh8N82tmlrrfm09AfpllA==", "ListOfRootCertificateIDs": {"RootCertificateID": [{"X509IssuerName": "<Name(CN=V2GRootCA,O=Switch,C=UK,DC=V2G)>", "X509SerialNumber": 12345}]}}}

[SwitchEV, OpenV2G v0.9.4] 800f02b4b2189c806610403c06104030d40060402040404608060140c10550c919c7a080604608e622460200c06aa080618129e8a9aa6eac4868264621e601a0c06aa0814180ca6eed2e8c6d0621660120c06aa080c2604aa96622660220c1413244d1327e458c802322c069e8a9a603c2e1a64686064646c60686a6c6a66b42e1a64706064646a60686a6c6a66b46092622860240c06aa080618169e8a9aa0e4deec86cae4e8621e601a0c06aa0814180ca6eed2e8c6d0621660120c06aa080c2604aa96622660220c1413244d1327e458c802322c069e8a9a60b260260c0e550c919c7a04020c10550c919c7a06020e06840008d73523723c3cc5978eeeeff651d989d1a6123e7cbab408d7bc6edf1ba42d3301c6c3e5111b1e3fa100f70397bfc43c0a5e34b4090321d3eeb71a030ece9f421746c060bc60180c06aa3a260203fe08046000601c0c06aa3a1e0203fe080806040710603a0c06aa3a1c082c08293bfcfcb36bf3cfebcade75ae4089ec84e1301bf8603e0c06aa3a460830602d00299f0dcd44b487a6ea586490ee69a7536e5df9d3a860140c10550c919c7a080604069000608a0442012566ad4b0f080d26e3e97c4adddc3f5bcd31842e2e3355e7ea0c660307d3617404405762680d2e5396ba863ca673ab6af6179950f86f9b5b34b5d6fcda7a03f4cb2802b3c4e616d6528434e3d563247526f6f7443412c4f3d5377697463682c433d554b2c44433d563247293e0b96017a00

[OpenV2G v0.9.5] 800f02b4b2189c806610403c06104030d40060402040404608060140c10550c919c7a080604608e622460200c06aa080618129e8a9aa6eac4868264621e601a0c06aa0814180ca6eed2e8c6d0621660120c06aa080c2604aa96622660220c1413244d1327e458c802322c069e8a9a603c2e1a64686064646c60686a6c6a66b42e1a64706064646a60686a6c6a66b46092622860240c06aa080618169e8a9aa0e4deec86cae4e8621e601a0c06aa0814180ca6eed2e8c6d0621660120c06aa080c2604aa96622660220c1413244d1327e458c802322c069e8a9a60b260260c0e550c919c7a04020c10550c919c7a06020e06840008d73523723c3cc5978eeeeff651d989d1a6123e7cbab408d7bc6edf1ba42d3301c6c3e5111b1e3fa100f70397bfc43c0a5e34b4090321d3eeb71a030ece9f421746c060bc60180c06aa3a260203fe08046000601c0c06aa3a1e0203fe080806040710603a0c06aa3a1c082c08293bfcfcb36bf3cfebcade75ae4089ec84e1301bf8603e0c06aa3a460830602d00299f0dcd44b487a6ea586490ee69a7536e5df9d3a860140c10550c919c7a080604069000608a0442012566ad4b0f080d26e3e97c4adddc3f5bcd31842e2e3355e7ea0c660307d3617404405762680d2e5396ba863ca673ab6af6179950f86f9b5b34b5d6fcda7a03f4cb2802b3c4e616d6528434e3d563247526f6f7443412c4f3d5377697463682c433d554b2c44433d563247293e0b9e00017a00

barsnick commented 6 months ago

(1) What I suspect is, "Certificate users MUST be able to handle serialNumber values up to 20 octets."([RFC5280] 4.1.2.2. Serial Number) is more natural and reasonable when it is changed to "Certificate users MUST be able to handle serialNumber values up to 20 digits." ie, number of '99999999999999999999' -> uint64 could cover it, what I meant is not "99999999999999999999" char strings.

Even if you think that's reasonable, that's not the common understanding of the definition, and you cannot just fiddle with than.

I agree with @SiebrenW's interpretation that we need up to 160 bits.

IMHO, OpenV2G v0.9.4(30 January 2018) is the stable release

I would go for 0.9.5: https://sourceforge.net/p/openv2g/code/119/ though it was never tagged in SVN.

I couldn't find where it(supporting big integer) from

Right here: https://sourceforge.net/p/openv2g/code/121/tree/trunk/src/codec/DecoderChannel.c#l277 (Note: 0.9.5 fixed potential buffer overflows in this code.)

but the latest OpenV2G generate different EXI stream(near end of stream, see below log) with SwitchEV(as I know SwitchEV already have coworked with the Hubject)

That is indeed an issue, what to consider as a reference.

barsnick commented 6 months ago

I was wondering why they didn't define a struct called bigInt or something, but I suppose it's hardly necessary for a single occerence.

xmldsig-core-schema.xsd unfortunately pulled in a lot of unfortunate XML "junk", and stuff that is only needed once, and a pain to support (see e.g. https://github.com/EVerest/cbexigen/issues/56).

kwoyhope commented 6 months ago

I agree that we'd better(should) to implement standard as possible as we could. I just want to remind that we also consider the interoperability issue might be caused. If new changes(cbV2G, support BigInteger) works fine for both short(under 32bit) and long(ex, 16 bytes long) Serifal Number, it could be OK with others.

SiebrenW commented 6 months ago

If we were to send this to the EV then I would agree, but considering the EVSE gets this X509SerialNumber from the EV and we don't send it, we have to handle anything the EV may send towards us. For interoperability supporting this with big int will not cause issues, smaller ints are still supported. If we don't and an EV sends something over 2^64 we're out of luck and the charge session will fail.

Putting this in risk factors: the risk not to support it is present. The risk to support it is maybe a bit of overhead (not as trivial to use this as a normal int, but we're not doing any operations on this int I hope), but otherwise an EV will not complain if we support bigger ints than i64.

SiebrenW commented 6 months ago

I tested with a python script using OpenEXI that checks for schema validity using lxml and encodes/decodes some EXI messages to/from XML. Using a value that wouldn't fit in an int64 the validator and encoder didn't complain at all. We can assume that EVs may add giant numbers as such. XML and V2GTP message:

<?xml version="1.0" encoding="UTF-8"?>
<ns0:CertificateInstallationReq xmlns:ns0="urn:iso:std:iso:15118:-20:CommonMessages">
  <ns1:Header xmlns:ns1="urn:iso:std:iso:15118:-20:CommonTypes">
    <ns1:SessionID>3030303030303030</ns1:SessionID>
    <ns1:TimeStamp>1707896956850052</ns1:TimeStamp>
  </ns1:Header>
  <ns0:OEMProvisioningCertificateChain ns0:Id="ID1">
    <ns0:Certificate>3q2+7w==</ns0:Certificate>
    <ns0:SubCertificates>
      <ns0:Certificate>ul66EQ==</ns0:Certificate>
    </ns0:SubCertificates>
  </ns0:OEMProvisioningCertificateChain>
  <ns0:ListOfRootCertificateIDs>
    <ns1:RootCertificateID xmlns:ns1="urn:iso:std:iso:15118:-20:CommonTypes">
      <ns2:X509IssuerName xmlns:ns2="http://www.w3.org/2000/09/xmldsig#">the name</ns2:X509IssuerName>
      <ns2:X509SerialNumber xmlns:ns2="http://www.w3.org/2000/09/xmldsig#">590295810358705651712</ns2:X509SerialNumber>
    </ns1:RootCertificateID>
  </ns0:ListOfRootCertificateIDs>
  <ns0:MaximumContractCertificateChains>2</ns0:MaximumContractCertificateChains>
  <ns0:PrioritizedEMAIDs>
    <ns0:EMAID>some emaid</ns0:EMAID>
  </ns0:PrioritizedEMAIDs>
</ns0:CertificateInstallationReq>
auto exi = "\x01\xFE\x80\x02\x00\x00\x00\x46\x80\x1c\x04\x18\x18\x18\x18\x18\x18\x18\x18\x08\x49\xfb\x4f\xba\xba\xa8\x40\x32\x02\xa4\xa2\x18\x80\x9b\xd5\xb7\xdd\xe0\x04\xba\x5e\xba\x11\x20\x0a\x74\x68\x65\x20\x6e\x61\x6d\x65\x08\x08\x08\x08\x08\x08\x08\x08\x08\x04\x01\x00\x80\x18\xe6\xde\xda\xca\x40\xca\xda\xc2\xd2\xc8\x40";

Back is also fine. Result:

<?xml version="1.0" encoding="UTF-8"?>
<s1:CertificateInstallationReq xmlns:s1="urn:iso:std:iso:15118:-20:CommonMessages"
                               xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
                               xmlns:xsd="http://www.w3.org/2001/XMLSchema"
                               xmlns:s0="http://www.w3.org/2000/09/xmldsig#"
                               xmlns:s2="urn:iso:std:iso:15118:-20:CommonTypes">
    <s2:Header>
        <s2:SessionID>3030303030303030</s2:SessionID>
        <s2:TimeStamp>1707896956850052</s2:TimeStamp>
    </s2:Header>
    <s1:OEMProvisioningCertificateChain s1:Id="ID1">
        <s1:Certificate>3q2+7w==</s1:Certificate>
        <s1:SubCertificates>
            <s1:Certificate>ul66EQ==</s1:Certificate>
        </s1:SubCertificates>
    </s1:OEMProvisioningCertificateChain>
    <s1:ListOfRootCertificateIDs>
        <s2:RootCertificateID>
            <s0:X509IssuerName>the name</s0:X509IssuerName>
            <s0:X509SerialNumber>590295810358705651712</s0:X509SerialNumber>
        </s2:RootCertificateID>
    </s1:ListOfRootCertificateIDs>
    <s1:MaximumContractCertificateChains>2</s1:MaximumContractCertificateChains>
    <s1:PrioritizedEMAIDs>
        <s1:EMAID>some emaid</s1:EMAID>
    </s1:PrioritizedEMAIDs>
</s1:CertificateInstallationReq>
kwoyhope commented 6 months ago

By the way, it seems that the certificaate of https(eg, *.google.com, below) already use BigInt for SerialNumber. it might not mean all OEMProvisioningCert(EV manufacturer) use BigInt for SerialNumber, but it's possible(already) in the field. image And the interoperability issue I have concerned is not just encoding/decoding problem, but if the fragment encoding exi stream of ExiCodecA(eg, SwitchEV) is not same(all bitwise) with the fragment encoding(not decoding) ExiCodecB(eg, OpenV2G v0.9.5), even though both ExiCodec can decode(parse) V2G message from other side, it may fail to verify the Digest(based on fragment exi stream bytes from each codec) of cetificate. (in this case(CertificationInstallationReq), performed by Seconday Actor(not SECC)) If I have a chance to ask EV Side(eg, Vector implementation) someday(in Testival?), I'd like to know what it(EV's EXI Codec) really implements. Thanks for your thorough investigation.

kwoyhope commented 6 months ago

FYI, I find that the "Serial number" of 'Hubject EU V2G Root CA G2'(https://www.hubject.com/download-pki) is also BigInt('BB BB BC 1A C4 5A D2 DF E5 B1 D8 73 C3 5C 84'), so I agree that we should support it. (the remaining question is the implementation of OpenV2G v0.9.5 is correct or not. (compared to SwitchEV's))