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

Vendor #3

Draft
wants to merge 409 commits into
base: main
Choose a base branch
from
Draft

Vendor #3

wants to merge 409 commits into from

Conversation

thealastair
Copy link
Collaborator

@thealastair thealastair commented Mar 23, 2021

The files are being generated in the wrong place (not in halter/xxx), we had a fix for this in our version that we'll need to bring in (or find out how to do it properly). Without this we get (for example)
notifications.h

#include "halter/requests.pb.h"

but the files aren't in the halter/ directory because it seems to ignore the package name when generating the path

This should help detect errors such as invalid pointer in decoded structure.
This is actually fully defined behavior, but it still makes sense to cast here.
This stops ridicuously large mallocs from getting through
on length-limited streams or buffers. Typically you should
also override realloc() to limit allocation size yourself
if dealing with untrusted data in pointer mode, but this
at least limits the potential denial-of-service attacks.
Seems to be somehow environment dependent, as it
only crashes with "Invalid argument" when run from
the Jenkins job.
PetteriAimonen and others added 25 commits January 22, 2021 08:33
For Bazel build system support.
This regression was caused by commit edf6dcb.

Added test coverage for it now.
…anopb#645)

Comments from .proto file are now included automatically when .proto
file is passed directly to nanopb_generator.py, and also when using
protoc --nanopb_out=. When using --descriptor_out=, --include_source_info
will need to be added to protoc command line.

Added a simple test case for the comment generation.
…anopb#645)

The old python-protobuf library didn't have __str__() handler for
repeated fields, so have to convert to list() first.
…nanopb#617)

Previous fix in commit ee1aff9 was incorrect.
The descriptor array should be kept ordered and generator is now fixed
to do so.
This partially reverts commit ee1aff9.

Trying to handle the issue in pb_common.c is misguided.
The generator has now been fixed to keep the descriptor array in sorted order.
Nanopb would call free() or realloc() on an invalid
(attacker controlled) pointer value when all the following
conditions are true:

- PB_ENABLE_MALLOC is defined at the compile time
- Message definition contains an oneof field, and the oneof
  contains at least one pointer type field and at least one
  non-pointer type field.
- Data being decoded first contains a non-pointer value for
  the oneof field, and later contains an overwriting pointer
  value.

Depending on message layout, the bug may not be exploitable in all
cases, but it is known to be exploitable at least with string and
bytes fields. Actual security impact will also depend on the heap
implementation used.
Previously added static_message member of the oneof
had the shortcoming that the first member of the submessage
was also a pointer. So when it aliased with a pointer to a message,
it was still a valid NULL pointer.
Also reorganized the script so that poetry build happens
in a separate build folder instead of polluting the working tree.

Including the generated file is a bit of a hack, but
better than the alternative of it not working when installed system-wide.
This is related to the fallback logic that is used for encoded
size calculations when dependent files cannot be found at generation
time.

Cache union and size definitions before writing it to file. Group
entries under same cpp_guard in dictionary. Duplicated entries will be
ignored due to set ignoring new entries. After that convert set to
list and sort it. It prevents mess when regenerating files.

Implementation by Jakub Tymejczyk in pull request nanopb#648,
slightly modified for brevity.
@thealastair thealastair marked this pull request as draft March 23, 2021 08:23
@shreyasbharath
Copy link

shreyasbharath commented Mar 23, 2021

It'd be nice to use the third party repo as is without creating our own repo?

If we fix that generation problem and submit it upstream, we can get rid of this repo.

@thealastair
Copy link
Collaborator Author

It'd be nice to use the third party repo as is without creating our own repo?

If we fix that generation problem and submit it upstream, we can get rid of this repo.

We would if it worked out of the box, maybe it does now

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

Successfully merging this pull request may close these issues.