{"action":"created","issue":{"url":"https://api.github.com/repos/RoaringBitmap/CRoaring/issues/116","repository_url":"https://api.github.com/repos/RoaringBitmap/CRoaring","labels_url":"https://api.github.com/repos/RoaringBitmap/CRoaring/issues/116/labels{/name}","comments_url":"https://api.github.com/repos/RoaringBitmap/CRoaring/issues/116/comments","events_url":"https://api.github.com/repos/RoaringBitmap/CRoaring/issues/116/events","html_url":"https://github.com/RoaringBitmap/CRoaring/issues/116","id":271974525,"number":116,"title":"security: roaring_bitmap_portable_deserialize() gives undefined behavior on evil input","user":{"login":"adamhooper","id":10812,"avatar_url":"https://avatars3.githubusercontent.com/u/10812?v=4","gravatar_id":"","url":"https://api.github.com/users/adamhooper","html_url":"https://github.com/adamhooper","followers_url":"https://api.github.com/users/adamhooper/followers","following_url":"https://api.github.com/users/adamhooper/following{/other_user}","gists_url":"https://api.github.com/users/adamhooper/gists{/gist_id}","starred_url":"https://api.github.com/users/adamhooper/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/adamhooper/subscriptions","organizations_url":"https://api.github.com/users/adamhooper/orgs","repos_url":"https://api.github.com/users/adamhooper/repos","events_url":"https://api.github.com/users/adamhooper/events{/privacy}","received_events_url":"https://api.github.com/users/adamhooper/received_events","type":"User","site_admin":false},"labels":[{"id":294068602,"url":"https://api.github.com/repos/RoaringBitmap/CRoaring/labels/help%20wanted","name":"help wanted","color":"159818","default":true},{"id":743616012,"url":"https://api.github.com/repos/RoaringBitmap/CRoaring/labels/security","name":"security","color":"d4c5f9","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2017-11-07T20:36:35Z","updated_at":"2017-11-08T04:16:02Z","closed_at":null,"author_association":"NONE","body":"I can't find a way to test that an input buffer is valid. Also, I can't specify the input buffer in `roaring_bitmap_portable_deserialize()`. It only accepts a `const char*` -- there's no accompanying `size_t`, which is _necessary_ to describe a buffer.\r\n\r\nAn evil person can craft short, evil arguments to `roaring_bitmap_portable_deserialize()` that convince CRoaring to read beyond its memory bounds and expose private data. That means I can't let an API client supply me with a RoaringBitmap to describe his or her integers.\r\n\r\nCRoaring should not read beyond the bounds of the buffer: it should produce an error instead. The simplest way to achieve this would be:\r\n\r\n1. Require a `size_t len` in `roaring_bitmap_portable_deserialize()`\r\n2. Disallow reading the buffer before the beginning of `buf` or after the end of `buf+len`.\r\n3. Make `roaring_bitmap_portable_deserialize()` return `NULL` if the input is invalid."},"comment":{"url":"https://api.github.com/repos/RoaringBitmap/CRoaring/issues/comments/342706105","html_url":"https://github.com/RoaringBitmap/CRoaring/issues/116#issuecomment-342706105","issue_url":"https://api.github.com/repos/RoaringBitmap/CRoaring/issues/116","id":342706105,"user":{"login":"K2","id":16319957,"avatar_url":"https://avatars3.githubusercontent.com/u/16319957?v=4","gravatar_id":"","url":"https://api.github.com/users/K2","html_url":"https://github.com/K2","followers_url":"https://api.github.com/users/K2/followers","following_url":"https://api.github.com/users/K2/following{/other_user}","gists_url":"https://api.github.com/users/K2/gists{/gist_id}","starred_url":"https://api.github.com/users/K2/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/K2/subscriptions","organizations_url":"https://api.github.com/users/K2/orgs","repos_url":"https://api.github.com/users/K2/repos","events_url":"https://api.github.com/users/K2/events{/privacy}","received_events_url":"https://api.github.com/users/K2/received_events","type":"User","site_admin":false},"created_at":"2017-11-08T04:16:02Z","updated_at":"2017-11-08T04:16:02Z","author_association":"NONE","body":"I can defiantly help out here, was just getting some personal projects time now the holidays are soon, there's a decent amount of places if were looking to harden the API.  If we assume 32 and 64 bit widths, that adds a lot of potential around the areas where we mix int and size_t with respect to promotion and truncation that can cause out of bounds or corruption. \r\n\r\nIs there anything else than the serialization that makes sense to protect? We don't claim to be MT safe so that makes life a bit easier. Would any of the bindings IPC their bitmaps around in some other way, pipe or something...?   I took a quick look at the Java implementation and it seems no JNI is used, I took an imported function that [CRoaring.NET](https://github.com/RogueException/CRoaring.Net/blob/5a6d5bb176c49fb96923450bb1fe6f120694a3b6/src/CRoaring.Net/NativeMethods.cs#L13) uses as an example of what we would need to change.\r\n\r\nWe would want to look at some optimized overflow routines, determine the best placement, here's how a friend of mine suggested a possible implementation (his blog post on the subject https://www.fefe.de/intof.html).\r\n\r\nMore or less, every place we allocate memory where we do not preserve the result of an expression derived in the scope of the parameter passed to the routine doing the allocation is an instant bug and has to be fixed.  It can be argued the other way but I've got plenty of reasons why it's a bug for security and performance at the same time (since you will be required to re-calculate that value repeatedly if you're trying to use that buffer safely) (second example that would need additional checking from GO binding https://github.com/RoaringBitmap/gocroaring/blob/master/roaring.c#L2835) \r\n\r\nThe main questions would be; \r\n- Do we know of other scenarios/threats to protect?\r\n- Make this an always on sort of thing? \r\n       Or have it opt-in? (rpmalloc did not want the perf hit so chose to optionally define it \r\n       https://github.com/rampantpixels/rpmalloc/issues/26)  \r\n\r\nThen just figure out the details of a good pattern we like for optimally handling the API, e.g. required changes to roaring_array.c\r\n\r\n```C\r\n\r\nint umult32(uint32_t a,uint32_t b,uint32_t* c) {\r\n    unsigned long long x=(unsigned long long)a*b;\r\n    if (x>0xffffffff) return 0;\r\n    *c=x&0xffffffff;\r\n    return 1;\r\n  }\r\n\r\nbool ra_init_with_capacity(roaring_array_t *new_ra, uint32_t cap) {\r\n    if (!new_ra) return false;\r\n    new_ra->keys = NULL;\r\n    new_ra->containers = NULL;\r\n    new_ra->typecodes = NULL;\r\n\r\n    new_ra->allocation_size = cap;\r\n    new_ra->size = 0;\r\n\r\n    uint32_t Header = sizeof(uint16_t) + sizeof(void *) + sizeof(uint8_t);\r\n    uint32_t neededSize;\r\n    \r\n    if(!umult32(cap, Header, &neededSize))\r\n        return false;\r\n\r\n    void *bigalloc = malloc(neededSize);\r\n\r\n    if( bigalloc == NULL ) return false;\r\n    new_ra->containers = (void **)bigalloc;\r\n    new_ra->keys = (uint16_t *)(new_ra->containers + cap);\r\n    new_ra->typecodes = (uint8_t *)(new_ra->keys + cap);\r\n    return true;\r\n}\r\n```\r\n"}}
{"actor":{"display_login":"K2"}}