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

Remove drand48() usage #1907

Open
PSUdaemon opened this Issue May 15, 2017 · 4 comments

Comments

Projects
None yet
5 participants
@PSUdaemon
Contributor

PSUdaemon commented May 15, 2017

Per Coverity:

"drand48()" should not be used for security related applications, as linear congruential algorithms are too easy to break.

@PSUdaemon PSUdaemon added this to the 8.0.0 milestone May 15, 2017

@PSUdaemon PSUdaemon self-assigned this May 15, 2017

@PSUdaemon PSUdaemon changed the title from Remove `drand48()` usage to Remove drand48() usage May 15, 2017

@zwoop

This comment has been minimized.

Show comment
Hide comment
@zwoop

zwoop May 15, 2017

Contributor

Specifically it found these new ones (although it's old code):

** CID 1375126:    (DC.WEAK_CRYPTO)
/tools/jtest/jtest.cc: 2762 in make_range_header(int, double, char *, int)()
/tools/jtest/jtest.cc: 2763 in make_range_header(int, double, char *, int)()


________________________________________________________________________________________________________
*** CID 1375126:    (DC.WEAK_CRYPTO)
/tools/jtest/jtest.cc: 2762 in make_range_header(int, double, char *, int)()
2756       int tmp[3];
2757     
2758       if (!rbuf || !size_limit)
2759         return;
2760     
2761       tmp[0] = gen_bfc_dist(dr - 1.0);
   CID 1375126:    (DC.WEAK_CRYPTO)
   "drand48()" should not be used for security related applications, as linear congruential algorithms are too easy to break.
2762       tmp[1] = ((int)(drand48() * 1000000)) % (tmp[0] - 1 - 0 + 1);
2763       tmp[2] = ((int)(drand48() * 1000000)) % (tmp[0] - 1 - 0 + 1) + tmp[1] + 100;
2764     
2765       if (tmp[0] > 100) {
2766         if (tmp[0] <= tmp[2]) {
2767           tmp[2] = tmp[0] - 1;
/tools/jtest/jtest.cc: 2763 in make_range_header(int, double, char *, int)()
2757     
2758       if (!rbuf || !size_limit)
2759         return;
2760     
2761       tmp[0] = gen_bfc_dist(dr - 1.0);
2762       tmp[1] = ((int)(drand48() * 1000000)) % (tmp[0] - 1 - 0 + 1);
   CID 1375126:    (DC.WEAK_CRYPTO)
   "drand48()" should not be used for security related applications, as linear congruential algorithms are too easy to break.
2763       tmp[2] = ((int)(drand48() * 1000000)) % (tmp[0] - 1 - 0 + 1) + tmp[1] + 100;
2764     
2765       if (tmp[0] > 100) {
2766         if (tmp[0] <= tmp[2]) {
2767           tmp[2] = tmp[0] - 1;
2768         }

** CID 1375120:    (DC.WEAK_CRYPTO)
/tools/jtest/jtest.cc: 2789 in make_random_url(int, double *, double *)()
/tools/jtest/jtest.cc: 2790 in make_random_url(int, double *, double *)()


________________________________________________________________________________________________________
*** CID 1375120:    (DC.WEAK_CRYPTO)
/tools/jtest/jtest.cc: 2789 in make_random_url(int, double *, double *)()
2783       snprintf(rbuf, size_limit, "Range: bytes=%lu-%lu\r\n", fd[sock].range_start, fd[sock].range_end);
2784     }
2785     
2786     static void
2787     make_random_url(int sock, double *dr, double *h)
2788     {
   CID 1375120:    (DC.WEAK_CRYPTO)
   "drand48()" should not be used for security related applications, as linear congruential algorithms are too easy to break.
2789       *dr = drand48();
2790       *h  = drand48();
2791     
2792       if (zipf == 0.0) {
2793         if (*h < hitrate) {
2794           *dr                      = 1.0 + (floor(*dr * hotset) / hotset);
/tools/jtest/jtest.cc: 2790 in make_random_url(int, double *, double *)()
2784     }
2785     
2786     static void
2787     make_random_url(int sock, double *dr, double *h)
2788     {
2789       *dr = drand48();
   CID 1375120:    (DC.WEAK_CRYPTO)
   "drand48()" should not be used for security related applications, as linear congruential algorithms are too easy to break.
2790       *h  = drand48();
2791     
2792       if (zipf == 0.0) {
2793         if (*h < hitrate) {
2794           *dr                      = 1.0 + (floor(*dr * hotset) / hotset);
2795           fd[sock].response_length = gen_bfc_dist(*dr - 1.0);

** CID 1375119:  Error handling issues  (CHECKED_RETURN)
/plugins/experimental/ts_lua/ts_lua_fetch.c: 285 in ts_lua_fetch_one_item()


________________________________________________________________________________________________________

I'm slightly torn on this, it's pretty benign IMO (since it's a test tool). And, in other places in this code, we already do

// coverity[dont_call]

So, I think at least for jtest.cc, we should just add this.

Contributor

zwoop commented May 15, 2017

Specifically it found these new ones (although it's old code):

** CID 1375126:    (DC.WEAK_CRYPTO)
/tools/jtest/jtest.cc: 2762 in make_range_header(int, double, char *, int)()
/tools/jtest/jtest.cc: 2763 in make_range_header(int, double, char *, int)()


________________________________________________________________________________________________________
*** CID 1375126:    (DC.WEAK_CRYPTO)
/tools/jtest/jtest.cc: 2762 in make_range_header(int, double, char *, int)()
2756       int tmp[3];
2757     
2758       if (!rbuf || !size_limit)
2759         return;
2760     
2761       tmp[0] = gen_bfc_dist(dr - 1.0);
   CID 1375126:    (DC.WEAK_CRYPTO)
   "drand48()" should not be used for security related applications, as linear congruential algorithms are too easy to break.
2762       tmp[1] = ((int)(drand48() * 1000000)) % (tmp[0] - 1 - 0 + 1);
2763       tmp[2] = ((int)(drand48() * 1000000)) % (tmp[0] - 1 - 0 + 1) + tmp[1] + 100;
2764     
2765       if (tmp[0] > 100) {
2766         if (tmp[0] <= tmp[2]) {
2767           tmp[2] = tmp[0] - 1;
/tools/jtest/jtest.cc: 2763 in make_range_header(int, double, char *, int)()
2757     
2758       if (!rbuf || !size_limit)
2759         return;
2760     
2761       tmp[0] = gen_bfc_dist(dr - 1.0);
2762       tmp[1] = ((int)(drand48() * 1000000)) % (tmp[0] - 1 - 0 + 1);
   CID 1375126:    (DC.WEAK_CRYPTO)
   "drand48()" should not be used for security related applications, as linear congruential algorithms are too easy to break.
2763       tmp[2] = ((int)(drand48() * 1000000)) % (tmp[0] - 1 - 0 + 1) + tmp[1] + 100;
2764     
2765       if (tmp[0] > 100) {
2766         if (tmp[0] <= tmp[2]) {
2767           tmp[2] = tmp[0] - 1;
2768         }

** CID 1375120:    (DC.WEAK_CRYPTO)
/tools/jtest/jtest.cc: 2789 in make_random_url(int, double *, double *)()
/tools/jtest/jtest.cc: 2790 in make_random_url(int, double *, double *)()


________________________________________________________________________________________________________
*** CID 1375120:    (DC.WEAK_CRYPTO)
/tools/jtest/jtest.cc: 2789 in make_random_url(int, double *, double *)()
2783       snprintf(rbuf, size_limit, "Range: bytes=%lu-%lu\r\n", fd[sock].range_start, fd[sock].range_end);
2784     }
2785     
2786     static void
2787     make_random_url(int sock, double *dr, double *h)
2788     {
   CID 1375120:    (DC.WEAK_CRYPTO)
   "drand48()" should not be used for security related applications, as linear congruential algorithms are too easy to break.
2789       *dr = drand48();
2790       *h  = drand48();
2791     
2792       if (zipf == 0.0) {
2793         if (*h < hitrate) {
2794           *dr                      = 1.0 + (floor(*dr * hotset) / hotset);
/tools/jtest/jtest.cc: 2790 in make_random_url(int, double *, double *)()
2784     }
2785     
2786     static void
2787     make_random_url(int sock, double *dr, double *h)
2788     {
2789       *dr = drand48();
   CID 1375120:    (DC.WEAK_CRYPTO)
   "drand48()" should not be used for security related applications, as linear congruential algorithms are too easy to break.
2790       *h  = drand48();
2791     
2792       if (zipf == 0.0) {
2793         if (*h < hitrate) {
2794           *dr                      = 1.0 + (floor(*dr * hotset) / hotset);
2795           fd[sock].response_length = gen_bfc_dist(*dr - 1.0);

** CID 1375119:  Error handling issues  (CHECKED_RETURN)
/plugins/experimental/ts_lua/ts_lua_fetch.c: 285 in ts_lua_fetch_one_item()


________________________________________________________________________________________________________

I'm slightly torn on this, it's pretty benign IMO (since it's a test tool). And, in other places in this code, we already do

// coverity[dont_call]

So, I think at least for jtest.cc, we should just add this.

@zwoop

This comment has been minimized.

Show comment
Hide comment
@zwoop

zwoop May 15, 2017

Contributor

Looking at some code, I think we can / should use this wrapper class consistently:

class InkRand
{
public:
  InkRand(uint64_t d);

  void seed(uint64_t d);
  inkcoreapi uint64_t random();
  double drandom();

private:
  uint64_t mt[312];
  int mti;
};

This implements the marsenne twister algorithm.

Contributor

zwoop commented May 15, 2017

Looking at some code, I think we can / should use this wrapper class consistently:

class InkRand
{
public:
  InkRand(uint64_t d);

  void seed(uint64_t d);
  inkcoreapi uint64_t random();
  double drandom();

private:
  uint64_t mt[312];
  int mti;
};

This implements the marsenne twister algorithm.

@igalic

This comment has been minimized.

Show comment
Hide comment
@igalic

igalic May 15, 2017

Contributor

Why does it implement that, rather than just using libcrypto's?

Contributor

igalic commented May 15, 2017

Why does it implement that, rather than just using libcrypto's?

@nikhil-khandelwal2

This comment has been minimized.

Show comment
Hide comment
@nikhil-khandelwal2

nikhil-khandelwal2 Oct 29, 2017

class InkRand
{
private:
uint64_t mt[312];
int mti;
public:
InkRand(uint64_t d);
void seed(uint64_t d);
inkcoreapi uint64_t random();
double drandom();
};

nikhil-khandelwal2 commented Oct 29, 2017

class InkRand
{
private:
uint64_t mt[312];
int mti;
public:
InkRand(uint64_t d);
void seed(uint64_t d);
inkcoreapi uint64_t random();
double drandom();
};

@bryancall bryancall modified the milestones: 8.0.0, 8.0.1 Sep 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment