Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

<X509SerialNumber /> not an integer #52

Open
moll opened this issue Jun 15, 2017 · 10 comments
Open

<X509SerialNumber /> not an integer #52

moll opened this issue Jun 15, 2017 · 10 comments
Assignees
Labels

Comments

@moll
Copy link

moll commented Jun 15, 2017

Hey,

Just a heads up that I too stumbled upon an issue with certificate serial numbers getting serialized in hex where they should be integers and noticed there's even a related TODO line (https://github.com/PeculiarVentures/xadesjs/blob/master/src/signed_xml.ts#L141) about. Docs signed with certs with such serial numbers obviously fail to validate outside of Xades.js. :)

@moll
Copy link
Author

moll commented Jun 15, 2017

Then again, this bug may be in Xmldsig.js. Not sure which bit does what here yet. ;)

@microshine
Copy link
Contributor

@moll Thank you for your issue. I'm waiting for new update of asn1.js which must allow to convert INTEGER values to BIG number. This update must resolve current problem

@microshine microshine added the bug label Jun 15, 2017
@microshine microshine self-assigned this Jun 15, 2017
@moll
Copy link
Author

moll commented Jun 15, 2017

Cool. Do you reckon there's any risk or unintended side-effect if I overwrite Xmldsig.X509Certificate.SerialNumber and have it return a string of a big number in the meanwhile?

@microshine
Copy link
Contributor

I thought about bn.js module to fix this error. But developer of asn1.js told me that he support INTEGER conversion to BIG NUMBER in 1 or 2 days.
And you are right. We have to update Xmldsig.X509Certificate.SerialNumber. I don't see any risk of changing this. There can be also added converter for SerialNumber attribute to check correct integer data.

@YuryStrozhevsky
Copy link

YuryStrozhevsky commented Jun 19, 2017

Now we the latest changes in ASN1.js and pvutils you would be able to get string representation like this:

const serialNumberRepresentation = certificate.serialNumber.valueBlock.toString();

Integer could be arbitrary in length.

@microshine
Copy link
Contributor

I published new versions of xmldsigjs and xadesjs. It returns integer in <SerialNumber>
@moll Could you check it?

@moll
Copy link
Author

moll commented Jun 20, 2017

Will do. Thank you! Did you fix the dependency bugs in those two, too, that I reported in other issues?

@microshine
Copy link
Contributor

no, I didn't

@moll
Copy link
Author

moll commented Jun 21, 2017

However PeculiarVentures/xmldsigjs@d9c2f90 seems to indicate you did fix PeculiarVentures/xmldsigjs#8. ;)

@moll
Copy link
Author

moll commented Jun 21, 2017

Having said that, you're right, xml-core is still missing as per #50.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants