Currently Being Moderated

In The Spotlight

 

Recently researchers discovered a security vulnerability in MySQL that allows users to log in without the correct password simply by trying repeatedly (CVE-2012-2122).  HD Moore explains some of the security implications of this "tragically comedic" vulnerability on his blog, including exploitability on several platforms and guidance for penetration testers.

 

The root cause was this code in sql/password.c:

 

474my_bool

475check_scramble(const char *scramble_arg, const char *message,

476               const uint8 *hash_stage2)

477{

478  SHA1_CONTEXT sha1_context;

479  uint8 buf[SHA1_HASH_SIZE];

480  uint8 hash_stage2_reassured[SHA1_HASH_SIZE];

481

482  mysql_sha1_reset(&sha1_context);

483  /* create key to encrypt scramble */

484  mysql_sha1_input(&sha1_context, (const uint8 *) message, SCRAMBLE_LENGTH);

485  mysql_sha1_input(&sha1_context, hash_stage2, SHA1_HASH_SIZE);

486  mysql_sha1_result(&sha1_context, buf);

487  /* encrypt scramble */

488    my_crypt((char *) buf, buf, (const uchar *) scramble_arg, SCRAMBLE_LENGTH);

489  /* now buf supposedly contains hash_stage1: so we can get hash_stage2 */

490  mysql_sha1_reset(&sha1_context);

491  mysql_sha1_input(&sha1_context, buf, SHA1_HASH_SIZE);

492  mysql_sha1_result(&sha1_context, hash_stage2_reassured);

CID 35852: At (1): Truncating the result of "memcmp" may cause it to be misinterpreted as 0. Note that "memcmp" may return an integer besides -1, 0, or 1.

493  return memcmp(hash_stage2, hash_stage2_reassured, SHA1_HASH_SIZE);

494}

 

Also critical for understanding this defect is the declaration of my_bool in my_global.h:

 

1073typedef char            my_bool; /* Small bool */

 

The man page for memcmp says that it can return an int less than, equal to, or greater than zero.  Since the return value is implicitly cast from int to char, the value will be truncated if it falls outside the range of [-128, 127].  Specifically, if the return value happens to be truncated to 0, the check_scramble function will appear to be returning a match between the password hashes even if they aren't a match.

 

To prove that some memcmp implementations actually behave this way I tried this tiny program:

 

#include <stdio.h>
#include <string.h>
int main() {
    int a = 16, b = 486;
    int rv = memcmp(&a, &b, 4);
    printf("rv = %d\n", rv);
    printf("(char)rv = %d\n", (char)rv);
    return 0;
}

 

Here's the output on my Mac:

 

     bash-3.2$ uname -mprs

     Darwin 11.4.0 x86_64 i386

     bash-3.2$ gcc test-memcmp.c

     bash-3.2$ ./a.out

     rv = -214

     (char)rv = 42

     bash-3.2$

 

The return values on this implementation of memcmp seem to be range bound such that truncation never creates a 0 value from a non-zero value, but the truncation can cause the return value to flip sign, as seen in this example.  In any case, the point is that developers should avoid the potential for truncation, and they cannot assume the return value is only -1, 0, or 1.

 

Lurking In the Shadows

 

For every defect in the spotlight, many others lurk in the dark background, yet to be discovered.

 

To bring more of these defects into the light, Donald Chai, one of our star analysis engineers, wrote a static analysis checker to find truncation of memcmp, strcmp, and strncmp return values, or cases where the return value was tested against constants other than 0.  Then we ran this checker over a corpus of 6286 open source projects, totaling over 330 million lines of code.  It took almost two days to build these packages, and less than an hour to analyze them all on our parallel condor cluster with ~20 nodes.

 

Overall we found 16 defects of this type, and no false positives.  One of these was the original MySQL vulnerability, and there were no other findings in MySQL (other than copies of the same password.c file in libraries like libmysql, libmysqld, and libmysql_r).

 

One caveat is that these open source packages were from a Debian 5.0 distribution.  Debian knows how to build all of these packages from source, which makes it convenient for static analysis.  But Debian 5.0 is also quite old, and many of the code bases are outdated.  We'll get around to updating this, but in the meantime for open source projects that put in a little effort they can benefit from live scanning of their changes via Coverity Scan.

 

The upshot is that many of these findings have already been fixed in later versions of the open source package.  We're only showing defects here that have either been fixed previously, where the open source developers have OK'd our blogging about it, or we're pretty confident they are not live security issues.

 

Nmap

 

We were a bit surprised to find this little gem in the code for Nmap-4.62 in osscan.cc:

 


806static void parse_classline(FingerPrint *FP, char *thisline, int lineno,

807                            int *classno) {

808  char *p, *q;

809

810// Wtf????

811  fflush(stdout);

812

CID 35868: Comparing the result of "strncmp" directly with "1" may cause it to be misinterpreted. Note that "strncmp" may return an integer besides -1, 0, or 1.

813  if (!thisline || strncmp(thisline, "Class ", 6) == 1) {

814    fatal("Bogus line #%d (%s) passed to %s()", lineno, thisline, __func__);

815  }

 

I love the Wtf (1), but it has little to do with the defect we found.  In this case, it's kind of hard to fathom what the developer was thinking.  Testing strncmp() == 1 is not only wrong for the positive case, but what if the return value is negative...?

 

Fortunately this was fixed, and here's what the code looks like as of Nmap-6.0.1:


1006  if (!thisline || strncmp(thisline, "Class ", 6) != 0)

1007    fatal("Bogus line #%d (%s) passed to %s()", lineno, thisline, __func__);

All better.

 

Nawm

 

One of the wonderful things about analyzing a huge amount of open source is that you can discover obscure bugs in obscure programs that are long forgotten.  Nawm is Not Another Window Manager - though it's not obvious what it is.  In any case it's long since unmaintained and probably has a near-zero user base.  Here's what this checker found in Nawm-0.0.20030130:

 

278      switch (op)

279        {

280        case LESS:

CID 35867: Comparing the result of "strcmp" directly with "-1" may cause it to be misinterpreted. Note that "strcmp" may return an integer besides -1, 0, or 1.

281          return strcmp((char *)left, (char *)right) == -1;

282          break;

283        case LESSEQUAL:

CID 35867: Comparing the result of "strcmp" directly with "-1" may cause it to be misinterpreted. Note that "strcmp" may return an integer besides -1, 0, or 1.

284          return strcmp((char *)left, (char *)right) != 1;

285          break;

286        case GREATER:

CID 35867: Comparing the result of "strcmp" directly with "-1" may cause it to be misinterpreted. Note that "strcmp" may return an integer besides -1, 0, or 1.

287          return strcmp((char *)left, (char *)right) == 1;

288          break;

289        case GREATEREQUAL:

CID 35867: Comparing the result of "strcmp" directly with "-1" may cause it to be misinterpreted. Note that "strcmp" may return an integer besides -1, 0, or 1.

290          return strcmp((char *)left, (char *)right) != -1;

291          break;

292        case EQUAL:

293          return strcmp((char *)left, (char *)right) == 0;

294          break;

295        case NOTEQUAL:

296          return strcmp((char *)left, (char *)right) != 0;

297          break;

298        }

299    }

300  /* can't happen */

301  return 0;

302}


This is a good example that illustrates why strcmp and friends have the return value they do.  It allows for the possibility of less-than and other comparisons between strings, not just the primary use of equality checking.(2)  The way I like to think about strcmp is the following:

 

   strcmp(a, b) CMP 0        <==>      a CMP b

 

where CMP is a comparison operator.  For example:

 

strcmp call"meaning"
strcmp(a, b) == 0"a == b"
strcmp(a, b) != 0"a != b"
strcmp(a, b) < 0"a < b"
strcmp(a, b) > 0"a > b"
strcmp(a, b) <= 0"a <= b"
strcmp(a, b) >= 0"a >= b"

 

I leave it as an exercise to the reader to fix the code from Nawm.

 

Olsrd

 

In this one the bug might cause a different packet route path than expected.  The resulting logic bug would be pretty difficult to pin down in testing or deployment, especially since it only appears on certain platforms:

 


434/**

435 * compare two route paths.

436 *

437 * returns TRUE if the first path is better

438 * than the second one, FALSE otherwise.

439 */

440static olsr_bool

441olsr_cmp_rtp(const struct rt_path *rtp1, const struct rt_path *rtp2, const struct rt_path *inetgw)

442{

443    olsr_linkcost etx1 = rtp1->rtp_metric.cost;

444    olsr_linkcost etx2 = rtp2->rtp_metric.cost;

445    if (inetgw == rtp1) etx1 *= olsr_cnf->lq_nat_thresh;

446    if (inetgw == rtp2) etx2 *= olsr_cnf->lq_nat_thresh;

447

448    /* etx comes first */

449    if (etx1 < etx2) {

450      return OLSR_TRUE;

451    }

452

453    /* hopcount is next tie breaker */

454    if ((etx1 == etx2) &&

455        (rtp1->rtp_metric.hops < rtp2->rtp_metric.hops)) {

456      return OLSR_TRUE;

457    }

458

459    /* originator (which is guaranteed to be unique) is final tie breaker */

CID 35859: Comparing the result of "memcmp" directly with "-1" may cause it to be misinterpreted. Note that "memcmp" may return an integer besides -1, 0, or 1.

460    if ((rtp1->rtp_metric.hops == rtp2->rtp_metric.hops) &&

461        (memcmp(&rtp1->rtp_originator, &rtp2->rtp_originator,

462                olsr_cnf->ipsize) == -1)) {

463      return OLSR_TRUE;

464    }

465

466    return OLSR_FALSE;

467}

 

We found the defect in olsrd 0.5.6, and it was already fixed in a later version.  I don't know how it was discovered, but I bet it was probably more painful than what I did.

 

Ncbi-tools

 

So far we've only seen cases where the return value is compared with a constant, but not truncated like in MySQL.  Here's an example from Ncbi-tools6, in build/netlib.c:

 

565static Int2 NEAR statCompare (NetStatPtr s1, NetStatPtr s2)

566{

567    if (s1 == NULL || s2 == NULL)

568        return 0;

569    if (s1->subsys < s2->subsys)

570        return -1;

571    if (s1->subsys > s2->subsys)

572        return 1;

CID 35865: Truncating the result of "strcmp" may cause it to be misinterpreted as 0. Note that "strcmp" may return an integer besides -1, 0, or 1.

573    return (Int2) StrCmp(s1->label, s2->label);

574}

 

And for those sticklers for detail, in various headers we have:

 

typedef short           Nlm_Int2, PNTR Nlm_Int2Ptr;
#define Int2            Nlm_Int2
#define Nlm_StrCmp      strcmp
#define StrCmp   Nlm_StrCmp

 

Here we have the return value of strcmp() being cast to short.  It's unclear if any implementations of strcmp() will return values outside the range of a short (in the MySQL example it was only char).  While security researchers and pen testers might want to know for exploitability, I don't think there's much reason for developers to have to worry about that when they can avoid the problem entirely by making a small change to the code.

 

Besides, there's more to worry about in Ncbi-tools:

 

371extern SegmenT feature2segment(SegmenT parent,

372                               const Char PNTR style_name,

373                               Int2 feature,

374                               Int4 left, Int4 top,

375                               Int4 label_width, Int4 image_width,

376                               Int4Ptr height)

377{

378  SegmenT   seg = NULL;

379  Int2      actual_style = Nlm_GetMuskCurrentSt();

CID 35864: Truncating the result of "strcmp" may cause it to be misinterpreted as 0. Note that "strcmp" may return an integer besides -1, 0, or 1.

380  Boolean   switch_style = (Boolean)StrCmp(style_name,

381                                           Nlm_GetMuskStyleName(actual_style));

 

In this case the "short defense" doesn't work:

 

typedef unsigned char   Nlm_Boolean, PNTR Nlm_BoolPtr;
#define Boolean         Nlm_Boolean

 

For good measure, here's a third:

 

CID 35862: Truncating the result of "strcmp" may cause it to be misinterpreted as 0. Note that "strcmp" may return an integer besides -1, 0, or 1.

1001    print_progress = (Boolean) StrCmp(myargs[9].strvalue, "F");

1002    if (print_progress)

1003       progress_file = myargs[9].strvalue;

 

It's hard to argue that these are security related defects in any way, but they might be functionality defects.  Ncbi-tools is used for creating biological data models, something I hope is done carefully!

 

OpenBabel

 

This one is fascinating because it show how developers often try to express what they mean, but end up with code that means something completely different to the machine:

 

 

168        str = temp_type;

169        if (strncmp(temp_type, "CL", 2) == 0) {

170          str = "Cl";

171        } else  if (strncmp(temp_type,"BR",2) == 0) {

172          str = "Br";

CID 35860: Comparing the result of "strncmp" directly with "2" may cause it to be misinterpreted. Note that "strncmp" may return an integer besides -1, 0, or 1.

173        } else if (strncmp(temp_type,"S.o2", 4) == 02) {

174          str = "S.O2";

175        } else if (strncmp(temp_type,"S.o", 3) == 0) {

176          str = "S.O";

177        } else if (strncmp(temp_type,"SI", 2) == 0) {

178          str = "Si";

 

The developer confused the "S.o2" string value with the return value 02, thinking that they wanted to test for the "same" value on both sides.  This is fixed in the latest OpenBabel, see GitHub.

 

Wine

 

So far we've found several fixed defects, now we'll move on to ones that were still in the latest versions.  In Wine, we found several cases, and here's one example:

 

256static HRESULT WINAPI RecycleBin_CompareIDs(IShellFolder2 *iface, LPARAM lParam, LPCITEMIDLIST pidl1, LPCITEMIDLIST pidl2)

257{

258    RecycleBin *This = (RecycleBin *)iface;

259

260    /* TODO */

261    TRACE("(%p, %p, %p, %p)\n", This, (void *)lParam, pidl1, pidl2);

262    if (pidl1->mkid.cb != pidl2->mkid.cb)

263        return MAKE_HRESULT(SEVERITY_SUCCESS, 0, pidl1->mkid.cb - pidl2->mkid.cb);

CID 10006: Truncating the result of "memcmp" may cause it to be misinterpreted. Note that "memcmp" returns a negative, zero, or positive result (not necessarily -1, 0, or 1).

264    return MAKE_HRESULT(SEVERITY_SUCCESS, 0, (unsigned short)memcmp(pidl1->mkid.abID, pidl2->mkid.abID, pidl1->mkid.cb));

265}

 

Thanks to a quick response from Amine Khaldi, a developer on ReactOS who also contributes back to Wine, we've confirmed this is an issue that will be fixed.  There were also 3 other findings in Wine but they were all in Win16 code, which according to Amine "rarely exists these days."  Still, they plan on fixing these cases.

 

Yersinia

 

Yersinia is a tool developed by pen testers, for pen testers.  We discovered one defect in the latest version:

 

268int8_t

269protocol_extra_compare(void *data, void *pattern)

270{

271   struct commands_param_extra *extra;

272

273   extra = (struct commands_param_extra *) data;

274   write_log(0, "comparando %ld con %ld\n", extra->id, (*(u_int32_t *)pattern));

CID 35858: Truncating the result of "memcmp" may cause it to be misinterpreted as 0. Note that "memcmp" may return an integer besides -1, 0, or 1.

275   return(memcmp(&extra->id, pattern, 4));

276}

 

Like the MySQL defect, this is a memcmp return value truncated by an implicit cast upon return.  According to the developer this will be fixed shortly.

 

It's interesting that when security people turn into developers they find that writing defect-free code is hard.  The security consultants and researchers I've interacted with who code substantially tend to understand developers better and have a stronger conceptual grasp of security and development issues.  All too often discussions among security professionals take on a decidedly snarky and demeaning tone towards developers (you know who you are).  Once you have code of your own, the game changes a bit...  Better to not cast stones.

 

Gambas

 

Developers are people, and people make mistakes in patterns that repeat, or at least rhyme.  This defect from Gambas 2.7 (also in the latest) is like a sibling to the one in MySQL.

 

Password checking function?  Check.

Use of comparison function?  MySQL uses memcmp, Gambas uses strcmp.

Truncation of return value?  Check.

 

112static bool check_crypt(const char *passwd, const char *crypted)

113{

114  char *result = crypt(passwd, crypted);

115

116  if (!result && errno == ENOSYS)

117    GB.Error("Crypting is not supported on this system");

118

119  if (!result)

120    return TRUE;

121  else

CID 35856: Truncating the result of "strcmp" may cause it to be misinterpreted as 0. Note that "strcmp" may return an integer besides -1, 0, or 1.

122    return strcmp(result, crypted);

123}

 

The cast from int to bool is actually safe if we're dealing with a "real" bool - e.g. in C++ or C99.  However it's not safe if the bool is a typedef to char, because of truncation:

 

#include <stdio.h>
#include <stdbool.h>
typedef char my_bool;
int main(int argc, char *argv[]) {
    printf("(bool)0xff00 = %d\n", (bool)0xff00);
    printf("(my_bool)0xff00 = %d\n", (my_bool)0xff00);
    return 0;
}

 

The output:


  bash-3.2$ gcc a.c

  bash-3.2$ ./a.out

  (bool)0xff00 = 1

  (my_bool)0xff00 = 0

  bash-3.2$

 

Therefore the semantics of a real bool are different from a synthetic one based on typedef, when it comes to casting.  To be ultra clear, here's the definition of bool in Gambas (gb_common.h):

 

91#if !defined(__cplusplus)

92

93  #ifndef FALSE

94    enum

95    {

96      FALSE = 0,

97      TRUE = 1

98    };

99  #endif

100

101  typedef

102    char boolean;

103

104  typedef

105    char bool;

106

107#endif

The developers of Gambas say that a fix is forthcoming.  Gambas is a relatively obscure basic-like programming language, and this code is an interface that is also obscure within Gambas, so the impact is probably very small in practice.

 

Final Tally

 

Here's a summary of the findings:

 

PackageDefectsFalse PositivesStatus
MySQL 5.010Fixed
Nmap 4.621

0

Fixed in 6.0.1 and probably earlier
Nawm 0.0.2003013010No longer maintained
Ncbi-tools 6.140Contacted, no fix available yet.
Openbabel 2.2.010Fixed in 2.3.0 and probably earlier.
Olsrd 0.5.610Fixed in 0.6.3 and probably earlier.
Yersinia 0.7.110Confirmed.  Patch forthcoming from developer.
Gambas 2.720Confirmed.  Patch forthcoming from developer.
Wine40Confirmed.  Patch forthcoming from developer.
All other 6000+ packages00No other defects detected (~330MLOC)
Total160

 

Developers can contribute to the solution

 

From a developer's perspective, it's simple.  When using comparison functions like memcmp and strcmp, don't truncate the return value -- and be especially careful when returning to a function with a smaller return type.

 

Better yet, always explicitly compare the return value of comparison functions, and never compare with anything other than 0.  The table, repeated here for good measure:

 

strcmp call"meaning"
strcmp(a, b) == 0"a == b"
strcmp(a, b) != 0"a != b"
strcmp(a, b) < 0"a < b"
strcmp(a, b) > 0"a > b"
strcmp(a, b) <= 0"a <= b"
strcmp(a, b) >= 0"a >= b"

 

Consistently using comparison functions with an explicit test against 0 is correct no matter what compiler, library implementation, or language variant you're using.

 

Security can help too

 

The standard advice for a vulnerability includes things like "don't put your MySQL server on the internet in the first place".  That kind of advice, while valuable, is directed at an IT administrator who is dealing with software that was developed months or years ago, passed through all manner of quality and security testing, and got shipped anyway.  It's advice that's late in the game, expensive to apply to millions of installations, and easily forgotten in the noise of myriad other requests.

 

We need more talk about bug birth control, if we're ever going to get beyond mopping up the mess they leave behind.

 

I'm hoping the security community evolves to emphasize buggy code as worthy of discussion.  It's true that many of these defects are in older code that's already fixed, or in software that is not widely used.  But that doesn't mean they're useless to review - they are helpful to developers to understand the kind of mistakes real developers make in real code.  If the developers of MySQL had seen salient examples of this defect before they wrote that fateful code, they might have avoided CVE-2012-2122.

 

Yes, looking at code is interesting from an exploitation perspective.  But what lessons do these defects have for developers?  What advice should we give them to avoid the same kind of problem occurring again and again?  This should become a fundamental part of the culture of disclosing defects, even ones that seem the same as ones we've seen before.  After all, remediation advice for IT has become repetitive too, but we still dispense it because the details matter, and if it's repeated often enough in conjunction with headlines, people begin to listen.

 

Thanks to Jon Passki, Scott McPeak, Chris Valasek, and Donald Chai for their review of this blog.

 

(1) I've often thought we could rank our defects by nearby WTF-like density.

(2) However given the potential for mistakes, and the "inverted" sense of the return value for equality comparisons, I sometimes feel this was a mistake in the design of the C standard library, and perhaps there should have been a string equality function for the common case and a separate, less commonly used function with a longer name to perform three-way comparisons.


Comments