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

CVE-2015-5237: Integer overflow in serialization #760

Closed
fweimer opened this issue Aug 27, 2015 · 10 comments
Closed

CVE-2015-5237: Integer overflow in serialization #760

fweimer opened this issue Aug 27, 2015 · 10 comments

Comments

@fweimer
Copy link

fweimer commented Aug 27, 2015

int is used to express the size of serialized messages. If the size exceeds 4 GiB, the application may allocate a buffer which is too small, or protobuf itself does this, in google::protobuf::MessageLite::SerializeToString. This lead to a heap buffer overflow, which may be exploitable for code execution in some cases.

It has been suggested that serialization of messages larger than 2 GiB is unsupported. But there is no good way for an application to ensure that the limit is not exceeded accidentally, without imposing rather draconian limits. To some degree, this is an gets-style interface.

Right now, this is more or less harmless because the message sizes involved are substantial. But this will change over time. My worry is that it will be difficult to fix this because some of the overflowing computations end up in generated *.pb.cc files, so the eventual fix will not be a simple library update.

@xfxyjwf
Copy link
Contributor

xfxyjwf commented Aug 27, 2015

The recommendation has been keeping the protobuf message relatively small (several MBs). There is code checking for sizes when parsing:
https://github.com/google/protobuf/blob/master/src/google/protobuf/io/coded_stream.h#L612

If you have a message that large (> 4GB), you might already be aware of this limit in parsing and have overridden the default limit. That's a chance to reconsider your design to avoid large messages.

@fweimer
Copy link
Author

fweimer commented Aug 27, 2015

@xfxyjwf, this issue is about serialization, not parsing. There is no default limit for serialization.

@xfxyjwf
Copy link
Contributor

xfxyjwf commented Aug 27, 2015

Do you mean the data is only serialized but never parsed back? I think parsing/serialization is always paired.

@fweimer
Copy link
Author

fweimer commented Aug 27, 2015

I'm not sure why you are asking this question. There is no serialized data to parse because the process crashes before in-memory serialization completes, due to the heap buffer overflow. In other words, this crash/security problem resides on the sending side, while constructing a protobuf message.

@xfxyjwf
Copy link
Contributor

xfxyjwf commented Aug 27, 2015

I was thinking in a more realistic case where the message is gradually grown to exceed 2G when the software evolves (i.e., you add more and more fields and data into the message). If you start straight out with a message larger 4G, it will fail lousily and I don't think anyone ship such software. A more likely case is that you ship a software which serializes/parses data close to 2G and some unusual user input pushes it to exceed protobuf limit. What I was saying is that in such cases, the developer should already be aware of the limit because protobuf parsing fails on a much smaller limit than 2G.

Anyway, for this issue, we can improve the documentation to make it explicit that protobuf does not support messages larger than 2G. We can probably also guard against >2G messages with CHECK errors in some cases, but that may not apply to all cases (e.g., >4G). Making protobuf to support >4G message is a non-goal.

@anderius
Copy link

@xfxyjwf: What was the resolution on this issue? Is there some limit in more recent versions of protobuf?

@xfxyjwf
Copy link
Contributor

xfxyjwf commented Oct 23, 2017

@anderius We now have a MessageLite::ByteSizeLong() method that will return the size in size_t instead of int and the serialization method is guaranteed to fail when ByteSizeLong() >= 2G.

@stephfonder
Copy link

In which release was this issue resolved?

@xfxyjwf
Copy link
Contributor

xfxyjwf commented Nov 16, 2017

@stephfonder It was first introduced in v3.4.0 release:
https://github.com/google/protobuf/releases/tag/v3.4.0

abergmeier pushed a commit to abergmeier/protobuf that referenced this issue May 15, 2019
…rotocolbuffers#760)

The marshaler, unmarshaler, and sizer functions are unused ever since
the underlying implementation was switched to be table-driven.
Change the function to only return the wrapper structs.

This change:
* enables generated protos to drop dependencies on certain proto types
* reduces the size of generated protos
* simplifies the implementation of oneofs in protoc-gen-go

Updates protocolbuffers#708
@RustyButtons
Copy link

@anderius We now have a MessageLite::ByteSizeLong() method that will return the size in size_t instead of int and the serialization method is guaranteed to fail when ByteSizeLong() >= 2G.

Hi I know this is and old issue, but i was hoping that you would clarify that i am right in thinking that this issue has been fixed by the two following things as quoted above,

Here we have the MessageLite::ByteSizeLong() function https://github.com/protocolbuffers/protobuf/blob/v3.1.0-alpha-1/src/google/protobuf/message_lite.cc#L240

And here we have a sanity check on the length of 'size' (ByteSizeLong()) > INT_MAX https://github.com/protocolbuffers/protobuf/blob/v3.1.0-alpha-1/src/google/protobuf/message_lite.cc#L247

Its is also mentioned that this is fixed in version 3.4.0, although if i am right in thinking that i have linked the correct fixing code, this was fixed in version 3.1.0-alpha-1 in this commit 98835fb

Is version 3.1.0-alpha-1 affected by CVE-2015-5237 ?

Many thinks

naemono added a commit to naemono/cloud-on-k8s that referenced this issue Jun 6, 2022
This reverts commit 3f7c4e9.

This CVE is a false positive anchore/grype#558

The CVE links to protocolbuffers/protobuf#760 , which is not the same as google.golang.org/protobuf
naemono added a commit to elastic/cloud-on-k8s that referenced this issue Jun 7, 2022
This reverts commit 3f7c4e9.

This CVE is a false positive anchore/grype#558

The CVE links to protocolbuffers/protobuf#760 , which is not the same as google.golang.org/protobuf
fantapsody pushed a commit to fantapsody/cloud-on-k8s that referenced this issue Feb 7, 2023
This reverts commit 3f7c4e9.

This CVE is a false positive anchore/grype#558

The CVE links to protocolbuffers/protobuf#760 , which is not the same as google.golang.org/protobuf
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

5 participants