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

Fall into an infinite loop when unpacking malformed data. #149

Closed
nori0428 opened this issue Oct 30, 2014 · 4 comments
Closed

Fall into an infinite loop when unpacking malformed data. #149

nori0428 opened this issue Oct 30, 2014 · 4 comments

Comments

@nori0428
Copy link
Contributor

When receiving malformed msgpack data from network, unpacker.next falls into an infinite loop.
And this occurs only when sizeof(size_t) == 4.

Please see https://gist.github.com/nori0428/55a63422add3e956bf68

Regards.

@redboltz
Copy link
Contributor

@nori0428 , Thank you for reporting the issue. I reproduced the problem.
As your patch indicated, the following algorithm is vulnerable:

    while(sz < size) {
        sz *= 2;
    }

I'm not the original version author but I can imagine the intention of the code. The algorithm calculate the minimum size that is multiple of page size and includes 'size'. The goal seems to be efficiency. However, if the minimum size is not found, I think that the request size is appropriate size.

If multiple of page size is mandatory, return NULL as your patch is good, but in this case, multiple of page size is not mandatory.

The algorithm tries to find such minimum size, if found, return the minimum size, otherwise return request size:

    while(sz < size) {
        size_t next_size = sz * 2;
        if (next_size < sz) {
            sz = size;
            break;
        }
        sz = next_size;
    }

By the way, I found similar codes the following location:

./erb/cpp03_zone.hpp.erb:229:        sz *= 2;
./include/msgpack/sbuffer.hpp:97:        while(nsize < m_size + len) { nsize *= 2; }
./include/msgpack/detail/cpp03_zone.hpp:274:        sz *= 2;
./include/msgpack/detail/cpp11_zone.hpp:272:        sz *= 2;
./include/msgpack/vrefbuffer.hpp:205:                nnext *= 2;
./include/msgpack/sbuffer.h:76:        while(nsize < sbuf->size + len) { nsize *= 2; }
./include/msgpack/unpack.hpp:1096:            next_size *= 2;
./include/msgpack/unpack.hpp:1111:            next_size *= 2;
./src/zone.c:82:        sz *= 2;
./src/unpack.c:399:            next_size *= 2;
./src/unpack.c:414:            next_size *= 2;
./src/vrefbuffer.c:182:            nnext *= 2;

They also should be fixed.

@nori0428
Copy link
Contributor Author

@redboltz , thank you for your quick response.

Certainly, similar codes you indicated are cause same problem.

By the way, I hope that msgpack::unpacker etc. have a method to set size limitation.

Now, msgpack::zone, sbuffer etc. try to allocate memory as much as possible.
I feel that it's little scary.
For example, when we receive uint32_array message and it indicates [0x0a, 0xaa, 0xaa, 0xaa] size, unpacker.next() try to allocate memory about ((size_t)-1 / 2) bytes.(and we can't notice it until receiving bad_alloc() exception etc.
Even as we have enough memory, it may cause other problems.(other processes can't allocate mem.)

So if a size limitation API (or define) exists, we can handle it properly.
I'm happy to consider this.

Regards.

@redboltz
Copy link
Contributor

I completely agree with you. A size limitation API is an important feature for protect a server. I will consider what kind of API is good.

@nobu-k
Copy link
Member

nobu-k commented Nov 16, 2014

Thank you for reporting the problem. It's partially fixed by #160, so msgpack-c won't fall into an infinite loop anymore. A remaining problem will be solved in #159. I close this issue.

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

3 participants