BREAKING CHANGE: disallow indefinite length, check length

[vasco-santos]

Hello,

libp2p/js-libp2p-crypto is using dannycoates/pem-jwk, which is using this module.

With the latest release, we got everything broken in libp2p. Can you revert the release and release as a breaking change, if you really want to go with this? I am not entirely sure about the change yet, but this is breaking all the projects now.

[indutny]

Sure thing. I've published 5.4.1 without it.

This is a breaking change, but it is a security bugfix. Ideally it should not become a major release of the library and stay as a minor or even a patch release. Would you mind sharing a test case that reproduces the problem?

FWIW, the crux is that DER encoding disallows certain inputs and asn1.js was very lenient about it before which leads to malleability. I suppose that one way this issue could surface at other libraries is that they were actually passing BER encoded data to the DER decoder. Is it something that happens in libp2p?

[vasco-santos]

Thanks very much @indutny

I was getting this error, if it helps:

I am not sure where it comes from so far

[indutny]

How can I reproduce it?

[vasco-santos]

The easiest way is to install peer-id https://github.com/libp2p/js-peer-id#createopts and do:

const PeerId = require('peer-id')
await PeerId.create()

(with the commit mentioned)

It will create a key using libp2p/js-libp2p-crypto

EDIT: If you use libp2p-crypto directly, you can do:

const crypto = require(libp2p-crypto)
await crypto.keys.generateKeyPair('RSA', 512)

FWIW, the crux is that DER encoding disallows certain inputs and asn1.js was very lenient about it before which leads to malleability. I suppose that one way this issue could surface at other libraries is that they were actually passing BER encoded data to the DER decoder. Is it something that happens in libp2p?

I need to check it out, I am not entirely familiar with libp2p/js-libp2p-crypto, where that should be implemented

[vasco-santos]

I believe the problem is in pem-jwk, which is the dependency that we use, more precisely on: https://github.com/dannycoates/pem-jwk/blob/master/index.js#L150-L159

[indutny]

Oh. I think it is OpenSSL that produces such PEM/DER in PEM_write_bio_RSAPrivateKey, and it is indirectly called through ursa. I'll have to investigate it in better detail.

[panva]

@indutny FWIW the RFC submodule test suites also fail for me when trying out this change.