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 index check on desc_table array access in runtime deserialization pony_deserialise_offset #4297

Open
masthoon opened this issue Jan 5, 2023 · 3 comments
Labels
bug Something isn't working needs discussion Needs to be discussed further needs investigation This needs to be looked into before its "ready for work"

Comments

@masthoon
Copy link

masthoon commented Jan 5, 2023

Inside pony_deserialise_offset, If no type is defined for the deserialisation, the type is read from the serialized stream:

uintptr_t id = *(uintptr_t*)((uintptr_t)ctx->serialise_buffer + offset);
t = desc_table[id];

The index id should be validated to be lower than the value of desc_table_size.

Note:

The deserialisation is already documented as unsafe because the deserialized object fields are never validated.

@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Jan 5, 2023
@SeanTAllen SeanTAllen added help wanted Extra attention is needed bug Something isn't working good first issue Good for newcomers labels Jan 5, 2023
@SeanTAllen
Copy link
Member

@SeanTAllen SeanTAllen added needs discussion Needs to be discussed further and removed help wanted Extra attention is needed good first issue Good for newcomers discuss during sync Should be discussed during an upcoming sync labels Jan 10, 2023
@SeanTAllen
Copy link
Member

We don't have any good way to deal with errors of this sort in the serialization of this sort. Some investigation of what we should do when we get unexpected values. Serialization was written for "totally trusted/may segfault". We probably want to throw an error but we need to verify that we are always good for doing it.

@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Jan 10, 2023
@SeanTAllen SeanTAllen added the needs investigation This needs to be looked into before its "ready for work" label Jan 10, 2023
@jemc
Copy link
Member

jemc commented Jan 10, 2023

What probably needs to happen for the error case is to call the "throw fn" passed to pony_deserialise with some code like this:

      serialise_cleanup(ctx);
      ctx->serialise_throw();
      abort();

@SeanTAllen SeanTAllen removed the discuss during sync Should be discussed during an upcoming sync label Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs discussion Needs to be discussed further needs investigation This needs to be looked into before its "ready for work"
Projects
None yet
Development

No branches or pull requests

4 participants