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

Missing validation around ObjectId and LibraryId #5

Open
GrabYourPitchforks opened this issue Mar 4, 2024 · 2 comments
Open

Missing validation around ObjectId and LibraryId #5

GrabYourPitchforks opened this issue Mar 4, 2024 · 2 comments

Comments

@GrabYourPitchforks
Copy link

GrabYourPitchforks commented Mar 4, 2024

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
Copy link
Owner

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 ;)

// From https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-nrbf/0a192be0-58a1-41d0-8a54-9c91db0ab7bf:
// "If the ObjectId is not referenced by any MemberReference in the serialization stream,
// then the ObjectId SHOULD be positive, but MAY be negative."
if (record.ObjectId != SerializationRecord.NoId)

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
Copy link
Owner

adamsitnik commented Mar 5, 2024

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:

// use Add on purpose, so in case of duplicate Ids we get an exception
recordMap.Add(record.ObjectId, record);

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

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

No branches or pull requests

2 participants