{"action":"opened","issue":{"url":"https://api.github.com/repos/awslabs/s2n/issues/805","repository_url":"https://api.github.com/repos/awslabs/s2n","labels_url":"https://api.github.com/repos/awslabs/s2n/issues/805/labels{/name}","comments_url":"https://api.github.com/repos/awslabs/s2n/issues/805/comments","events_url":"https://api.github.com/repos/awslabs/s2n/issues/805/events","html_url":"https://github.com/awslabs/s2n/issues/805","id":337765561,"node_id":"MDU6SXNzdWUzMzc3NjU1NjE=","number":805,"title":"Off by one bug in s2n_public_random()","user":{"login":"Sc00bz","id":2030592,"node_id":"MDQ6VXNlcjIwMzA1OTI=","avatar_url":"https://avatars0.githubusercontent.com/u/2030592?v=4","gravatar_id":"","url":"https://api.github.com/users/Sc00bz","html_url":"https://github.com/Sc00bz","followers_url":"https://api.github.com/users/Sc00bz/followers","following_url":"https://api.github.com/users/Sc00bz/following{/other_user}","gists_url":"https://api.github.com/users/Sc00bz/gists{/gist_id}","starred_url":"https://api.github.com/users/Sc00bz/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Sc00bz/subscriptions","organizations_url":"https://api.github.com/users/Sc00bz/orgs","repos_url":"https://api.github.com/users/Sc00bz/repos","events_url":"https://api.github.com/users/Sc00bz/events{/privacy}","received_events_url":"https://api.github.com/users/Sc00bz/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":0,"created_at":"2018-07-03T06:47:30Z","updated_at":"2018-07-03T06:47:30Z","closed_at":null,"author_association":"NONE","body":"**Problem:** \r\nThe input variable to the function `s2n_public_random` is called `max` but is used as count. https://github.com/awslabs/s2n/blob/643976c1c03c7f4a3003d7e066f18536c410c2b4/utils/s2n_random.c#L166\r\n\r\n**Proposed Solution:** \r\nDo one of the following:\r\n* Rename variable from `max` to `count`.\r\n* Add a comment to state it deviates from the norm and returns [0, max).\r\n* Change the function to the following, but changing the function will have unexpected consequences in other code that uses this. This probably should not be done. Unless you deprecate the function and add a new one like the following.\r\n```\r\nint64_t s2n_public_random(int64_t max)\r\n{\r\n    uint64_t r;\r\n+    uint64_t count = (uint64_t) max + 1;\r\n\r\n    gt_check(max, 0);\r\n\r\n    while (1) {\r\n        struct s2n_blob blob = {.data = (void *)&r, sizeof(r) };\r\n        GUARD(s2n_get_public_random_data(&blob));\r\n\r\n        /* Imagine an int was one byte and UINT_MAX was 256. If the\r\n         * caller asked for s2n_random(129, ...) we'd end up in\r\n         * trouble. Each number in the range 0...127 would be twice\r\n         * as likely as 128. That's because r == 0 % 129 -> 0, and\r\n         * r == 129 % 129 -> 0, but only r == 128 returns 128,\r\n         * r == 257 is out of range.\r\n         *\r\n         * To de-bias the dice, we discard values of r that are higher\r\n         * that the highest multiple of 'max' an int can support. If\r\n         * max is a uint, then in the worst case we discard 50% - 1 r's.\r\n         * But since 'max' is an int and INT_MAX is <= UINT_MAX / 2,\r\n         * in the worst case we discard 25% - 1 r's.\r\n         */\r\n-        if (r < (UINT64_MAX - (UINT64_MAX % max))) {\r\n-            return r % max;\r\n+        if (r < (UINT64_MAX - (UINT64_MAX % count))) {\r\n+            return r % count;\r\n        }\r\n    }\r\n}\r\n```"}}
{"actor":{"display_login":"Sc00bz"}}