adamsitnik / SafePayloadReader

MIT License
1 stars 0 forks source link

Missing validation around ObjectId and LibraryId #5

Open GrabYourPitchforks opened 8 months ago

GrabYourPitchforks commented 8 months ago

According to the MS-NRBF spec, ObjectId and LibraryId values must be positive 32-bit integers. Currently these values are read with no validation.

Additionally, the current behavior for the reader is that if it encounters multiple objects / libraries with the same id in the serialization stream, any records which appear later in the stream override records which appear earlier in the stream. This is an example of an opinionated behavior. Is this intentional? What are the consequences of allowing this?

adamsitnik commented 8 months ago

ObjectId and LibraryId values must be positive 32-bit integers

This was my initial assumption, but it turned out that ObjectIds that are referenced by other objects can be negative ;)

https://github.com/adamsitnik/SafePayloadReader/blob/cfb7ac5e388fcfbe4e24fd034c282509b0a283aa/System.Runtime.Serialization.BinaryFormat/SafePayloadReader.cs#L191-L194

I've hit that case when implementing some more sophisticated test.

I'll carefully study the spec for each record type and add the validation

adamsitnik commented 8 months ago

any records which appear later in the stream override records which appear earlier in the stream

If an object with given Id has been already recorded, the Add method is going to throw:

https://github.com/adamsitnik/SafePayloadReader/blob/cfb7ac5e388fcfbe4e24fd034c282509b0a283aa/System.Runtime.Serialization.BinaryFormat/SafePayloadReader.cs#L196-L197

I need to write tests for invalid inputs to have it covered.