pc-dart icon indicating copy to clipboard operation
pc-dart copied to clipboard

Is the asn1utils value start position calculated correctly?

Open domindx opened this issue 3 years ago • 9 comments

Hi, Shouldn't the condition be rather less or equal here? https://github.com/bcgit/pc-dart/blob/7aebcf232f047bb3a4f549ca0b53f14da40d90b8/lib/asn1/asn1_utils.dart#L17

Similarly as in decodeLength() method below.

domindx avatar Mar 29 '21 17:03 domindx

@mwcw @AKushWarrior He seems to be right I think. Can you double check it? After that we can create a PR.

Ephenodrom avatar Mar 30 '21 11:03 Ephenodrom

Hi,

It is calculating the position of the start of the data.

However it is making an assumption that the tag is only one byte long, tags can be longer, this should be a TODO.

MW

mwcw avatar Mar 31 '21 03:03 mwcw

Hi again, I'll add some background.

I've observed the issue when using https://pub.dev/packages/basic_utils package which lately migrated from asn1lib to pointycastle library.

The issue was with parsing one of my PEM files which has exactly 127 bytes long sequence inside. The effect was that the parser was trying to read data outside the buffer.

Quick fix which worked for this pem was to change the condition above to inclusive.

domindx avatar Mar 31 '21 06:03 domindx

@domindx Can you provide this PEM so I can add this later as a unit tests to my "basic utils" package?

Some background information: I tested the parsing of PEM files with nearly 50k certificates issued by DigiCert. So therefore I am quite sure it works in 99% of all cases ;).

Ephenodrom avatar Mar 31 '21 06:03 Ephenodrom

@Ephenodrom Sorry for lack of response, I was quite busy lately.

Unfortunately I cannot share the PEM, but I'll try to generate some example on which it may fail.

One thing which I want to add is that basic_utils version which is using asn1lib is working perfectly with mentioned PEM. Do you know the reasons why basic_utils migrated to pointycastle?

domindx avatar Apr 21 '21 07:04 domindx

@domindx The main reason why I switched from asn1lib to Pointycastle was to cut down the amount of dependencies of the basic utils package.

I completely checked the new ASN1 implementation with all unit tests i had in the basic utils package and I wrote a lot of new unit tests in the Pointycastle package.

But as always, there is one special scenario that is not yet tested ...

Ephenodrom avatar Apr 21 '21 08:04 Ephenodrom

Hello again, I wasn't able to prepare faulty PEM file, but I've added UT scenario which cover the issue I am encountering.

So in my case when the sequence have 7F length the start position seems to be calculated in the way that library is trying to read data outside the buffer.

From my understanding the start position in this case should be the same as for length below 7F. And actually it worked for me when I've made such change.

Please have a look at my commit: 61120af1ccb9a22a67895797c77a16a5818c4e45

domindx avatar May 13 '21 20:05 domindx

@mwcw I created a PR to fix the calculation of the start position.

Ephenodrom avatar Jun 28 '21 11:06 Ephenodrom

@mwcw Any information about the release date of the next version :)?

Ephenodrom avatar Jun 29 '21 14:06 Ephenodrom