bugFreeciv - Bugs: bug #18170, auth md5 sum broken on big-endian...

 
 
Show feedback again

bug #18170: auth md5 sum broken on big-endian machines (e.g., sparc64)

Submitted by:  Michal Mazurek <akfaew>
Submitted on:  Sun 29 May 2011 11:31:09 AM UTC  
 
Category: generalSeverity: 4 - Important
Priority: 5 - NormalStatus: Fixed
Assigned to: Jacob Nevins <jtn>Open/Closed: Closed
Release: 2.2.5,2.3.0-beta4Operating System: Any
Planned Release: 2.2.6,2.3.0,2.4.0

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Fri 24 Jun 2011 07:15:16 PM UTC, SVN revision 19851:

Replace our MD5 implementation with a more portable one by "Solar Designer", to
fix a problem where existing users would not have been able to log in to
big-endian servers (such as sparc64) if authentication was enabled.

Also, when creating new users, use Freeciv's MD5 implementation rather than
relying on the SQL server having one.

Reported by akfaew@gna.

See gna bug #18170.

(Browse SVN revision 19851)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Fri 24 Jun 2011 07:14:03 PM UTC, SVN revision 19850:

Replace our MD5 implementation with a more portable one by "Solar Designer", to
fix a problem where existing users would not have been able to log in to
big-endian servers (such as sparc64) if authentication was enabled.

Also, when creating new users, use Freeciv's MD5 implementation rather than
relying on the SQL server having one.

Reported by akfaew@gna.

See gna bug #18170.

(Browse SVN revision 19850)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Fri 24 Jun 2011 07:12:51 PM UTC, SVN revision 19849:

Replace our MD5 implementation with a more portable one by "Solar Designer", to
fix a problem where existing users would not have been able to log in to
big-endian servers (such as sparc64) if authentication was enabled.

Also, when creating new users, use Freeciv's MD5 implementation rather than
relying on the SQL server having one.

Reported by akfaew@gna.

See gna bug #18170.

(Browse SVN revision 19849)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Fri 24 Jun 2011 12:42:38 AM UTC, comment #15:

I've now set up a MySQL database and tested before and after my patch with all of trunk/S2_3/S2_2, creating new users (and manually checking the MD5 that ends up in the database) and checking login success and failure of existing ones. All looks in order and I can't tell the difference without and with my patch (which is as expected on my x86_64 machine).

So, will commit this soon unless anyone objects.

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Wed 22 Jun 2011 08:47:36 PM UTC, comment #14:

Attached new patches without the believed-unnecessary bug compatibility mode. These just change the MD5 implementation.

(They also move the creation of the MD5 sum that's saved in the database from new users from SQL to C.)

The S2_3 version backports cleanly to S2_2, and this is less invasive than I thought it would be, so I'm inclined to put this in 2.2.6.

In principle these are candidates for committing. However, I haven't yet tested them (beyond verifying the MD5 calculation) as I haven't got round to setting up a MySQL database as the backend for authentication. Such testing (involving creating a new user and logging in as an existing user) is probably a prerequisite for committing.

(file #13317, file #13318)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Tue 21 Jun 2011 10:26:36 PM UTC, comment #13:

New development: it turns out that when new users are created by freeciv-server when the --Newusers option is specified, the password stored in the database is hashed by MySQL's MD5 implementation, not Freeciv's. See the short-lived bug #18261.

If so, then there are no polluted databases out there, and the bug-compatibility mode isn't needed; nor is the new "hash" parameter in the conf file. We just need a new MD5 implementation, which I'm more or less done with.

However, if this is true, the bug then becomes that login after user creation can never have been possible on big-endian servers -- if we assume the password in the database is hashed correctly (either by freeciv-server or by a bespoke registration process), then at auth-existing-user time, Freeciv will generate the wrong hash for comparison and not let any user log in.

Question for akfaew: how did you discover this bug? Was it in the course of investigating an actual problem, and if so, what was the problem?

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Tue 21 Jun 2011 09:04:29 PM UTC, comment #12:

(For reference, attached a diff -- file #13307 -- that shows approximately the violence I did to Solar Designer's original code to implement bug compatibility.)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Tue 21 Jun 2011 08:15:57 PM UTC, comment #11:

> On a sparc64: [...]

Hurrah, looks good.

> The bool swap part is awfully ugly.

Well, the code is ugly because its purpose is ugly, IMO. Suggestions for deuglification welcome. (I'll probably rename the 'swap' parameter to 'evil_bug_compatibility_mode' or similar, to try to make it clearer there is mischief afoot.)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Tue 21 Jun 2011 10:25:00 AM UTC, comment #10:

On a sparc64:

2: AI*5 has been added as Easy level AI-controlled player.

>9743a66f914cc249efca164485a19c5c<
>2278ce9519b2223ecb2e2a2237e6d006<

2: Now accepting new client connections.

The bool swap part is awfully ugly.

Michal Mazurek <akfaew>
Thu 16 Jun 2011 08:21:24 PM UTC, comment #9:

Attached a WIP patch. This isn't a candidate for committing.

As threatened, I've replaced Ulrich Drepper's MD5 code with Solar Designer's. I've implemented a controllable bug-compatibility mode for old databases from big-endian servers, but I've not plumbed it in to be configurable from the database config file yet. There's also other tidying still to be done, and porting to S2_3.

I've verified that it gives correct and correctly-wrong answers in the test case from comment #1 (with the attached test patch, which spits out answers when the server is run):

@akfaew: Can you test this patch on your sparc64 machine? I've only tested this on my little-endian x86_64. It should be portable, but... You should see the same result as I did above. Patch applies to latest trunk.

(file #13231, file #13232)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Tue 14 Jun 2011 11:54:59 PM UTC, comment #8:

> I can't find a ticket for its addition to Freeciv, so I don't
> know more about its exact provenance (a "Mike Kaufman" was
> involved).


For the record: Mike & others originally wrote authentication code for freeciv pubserver only (now, how many of you even remember pubserver...:-) It was not in public freeciv version control, but just local patch applied when freeciv server for pubserver was built. It's quality reflected that; it was meant to run only on that one specific hardware and environment (hardcoded mysql usernames, passwords, filesystem paths...)
At some point Mike decided to save the authentication code from failing pubserver by adding it to freeciv version control.
It was behind #ifdefs and didn't even compile when I decided to set up my own public server. So I cleaned the code up a good deal to make it reusable in my case.

Marko Lindqvist <cazfi>
Project Administrator
Mon 13 Jun 2011 08:47:39 PM UTC, comment #7:

> My inclination is to find a more portable implementation of
> md5sum, rather than trying to fix up this one.

A quick Google finds this one, which seems to fit the bill:
http://openwall.info/wiki/people/solar/software/public-domain-source-code/md5

Any objections?

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Mon 13 Jun 2011 12:07:37 AM UTC, comment #6:

> Such an option is really rarely needed, and really shouldn't be
> used by accident, so I wonder if it should be commandline-option
> at all or only in database access configuration file (I haven't
> checked new authentication database access code, but I assume
> that server name and port, mysql user name and password are
> still read from configuration file).

Didn't know about that. Yes, that still exists on trunk in much the same form, I think (it's specified with "--Database <file>" rather than "--auth <file>" now).

It could take the form of a new "hash" parameter in that file, with options "md5" (default) and "md5swap" to solve this problem, but extensible to other hashes such as SHA-1 in future if we feel the need.

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Fri 03 Jun 2011 10:20:29 PM UTC, comment #5:

> How about a server option for bug-compatibility, allowing server
> operators who are afflicted with such databases to carry on?


That's what I was about to propose. This gives server operators the freedom to decide when they make the switch to new md5. They can do it immediately, or to postpone all the way until they migrate to big-endian machine. Could we actually provide option that has two values: one (strongly preferred default) which produces results similar to old little-endian behavior on all machines, and another which produces results similar to old big-endian behavior on all machines?
Such an option is really rarely needed, and really shouldn't be used by accident, so I wonder if it should be commandline-option at all or only in database access configuration file (I haven't checked new authentication database access code, but I assume that server name and port, mysql user name and password are still read from configuration file). Putting it to database itself would require adding completely new table as there is no kind of metadata table at the moment (no any one-row table). Then again, we may want such an table in the future anyway.

Whether this should be resolved in S2_3 depends a lot of how invasive our final solution is, and when someone makes it. This would be nice to fix before 2.3.0, but I don't consider it blocker. Potential server operators should now raise their voice and demand this to be fixed for 2.3.0 ("I'm going to create completely new database to be used with 2.3.0 and beyond, and it should be possible to make it right from the beginning")

Marko Lindqvist <cazfi>
Project Administrator
Fri 03 Jun 2011 09:49:10 PM UTC, comment #4:

Hmm.

Unfortunately it's not possible to provide a one-time procedure to fix the hashed passwords in corrupted databases, by the nature of the one-way hash.

I think the SWAP() macro is only used on input -- the hash is still MD5, it's just that its input octets are in a funny order.
So it would be technically possible to have the server hash the password in both the "wrong" and "correct" ways and accept either, but obviously that would reduce security a bit on every server (a lucky incorrect password would be accepted).

How about a server option for bug-compatibility, allowing server operators who are afflicted with such databases to carry on? Or a flag which can be put in the database itself?

Or we could just carry on with the status quo, ugly as it is... this shifts the trouble to people migrating their servers to new hardware.

We can't know how many server operators use big-endian machines, but I'd expect the majority to be Intel (little-endian). Still, we know at least one operator (akfaew) who's been bitten :/

(Do we want to do something about this for 2.3.0?)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Fri 03 Jun 2011 09:39:17 AM UTC, comment #3:

> Conversely, the effect of fixing this bug is that any big-endian
> server operators will effectively lose all their passwords
> (unless we put in a backwards-compatibility wart). Probably not
> one to drop into a stable release update, then.


Actually, as a server operator I'd hate to lose my user base even between major releases. Also, in the past, and maybe in the future, I've run several versions concurrently using same database. Some players are "locked" to certain versions (one that comes with their distribution), others switch between versions depending who (of the "locked" players) they play against.

Marko Lindqvist <cazfi>
Project Administrator
Fri 03 Jun 2011 01:04:17 AM UTC, comment #2:

Can't reproduce on my x86_64 machine -- the code gives the right answer for the example given. A possibly relevant difference is that sparc64 is big-endian whereas x86_64 is little-endian.

In fact, I'm pretty sure that's what it is. This code relies on dubious casts between octets and uint32s, and to this end it defines a SWAP() macro that depends on whether WORDS_BIGENDIAN is defined. Only there's no reason for that ever to be defined in the Freeciv codebase (the magic to work it out is protected by _LIBC, which I doubt is defined for us and wouldn't work anyway). So, I think it can only ever work on little-endian machines.

Probable proof of the pudding: if I define WORDS_BIGENDIAN on my little-endian machine, create_md5sum() gives the same result as the erroneous one in comment #0.

My inclination is to find a more portable implementation of md5sum, rather than trying to fix up this one.

(For the record, before I realised the above I had a look at what I think is the upstream version (git history); there's not been any significant change since 2005, and I don't see significant differences in the number-crunching bits between ours and theirs.)

I assume the effect of this bug is that authentication databases may not be portable from one server to another of a different architecture; in this case the server operator will effectively lose all their passwords. I don't think it affects interoperability between clients and servers?

Conversely, the effect of fixing this bug is that any big-endian server operators will effectively lose all their passwords (unless we put in a backwards-compatibility wart). Probably not one to drop into a stable release update, then.

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Wed 01 Jun 2011 09:18:00 AM UTC, comment #1:

MD5 in Freeciv is used for user authentication in the server (not an area into which I've ventured before, as it involves faffing with databases).

Our MD5 implementation appears to have been pinched from glibc in around 2005 (r10768). I can't find a ticket for its addition to Freeciv, so I don't know more about its exact provenance (a "Mike Kaufman" was involved). Still, if as I suspect it's a 64-bit cleanliness issue in the code, it might be possible to find the same bug in glibc's development history and possibly lift a newer version from glibc.

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Sun 29 May 2011 11:31:09 AM UTC, original submission:

1205 char checksum[DIGEST_HEX_BYTES + 1];
...
1210 create_md5sum("dupa", 4, checksum);
1211 fprintf(stderr, ">%s<\n", checksum);
produces:

>2278ce9519b2223ecb2e2a2237e6d006<


[12:48:ttypf][akfaew@spock:~/longturn/freeciv:4]$ echo -n dupa | md5
9743a66f914cc249efca164485a19c5c

Michal Mazurek <akfaew>

 

(Note: upload size limit is set to 1024 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #13317:  trunk-md5-portability.diff added by jtn (34kB - text/x-diff - trunk/S2_3/S2_2 r19832)
file #13318:  S2_3-S2_2-md5-portability.diff added by jtn (34kB - text/x-diff - trunk/S2_3/S2_2 r19832)
file #13307:  md5-freeciv.diff added by jtn (15kB - text/x-diff - comparison to Solar Designer's original code)
file #13231:  trunk-md5-WIP.diff added by jtn (33kB - text/x-diff - trunk r19768 -- WIP)
file #13232:  trunk-md5-test-hack.diff added by jtn (733B - text/x-diff - trunk r19768 -- WIP)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by cazfi (Posted a comment)
  • -unavailable- added by jtn (Posted a comment)
  • -unavailable- added by akfaew (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 16 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 24 Jun 2011 07:16:39 PM UTCjtnStatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Wed 22 Jun 2011 08:47:36 PM UTCjtnAttached File-=>Added trunk-md5-portability.diff, #13317
      Attached File-=>Added S2_3-S2_2-md5-portability.diff, #13318
      StatusIn Progress=>Ready For Test
      Planned Release2.3.0,2.4.0=>2.2.6,2.3.0,2.4.0
    Tue 21 Jun 2011 08:58:57 PM UTCjtnAttached File-=>Added md5-freeciv.diff, #13307
    Thu 16 Jun 2011 08:21:24 PM UTCjtnAttached File-=>Added trunk-md5-WIP.diff, #13231
      Attached File-=>Added trunk-md5-test-hack.diff, #13232
      CategoryNone=>general
      Release=>2.2.5,2.3.0-beta4
      Operating SystemNone=>Any
      Planned Release=>2.3.0,2.4.0
      Summarymd5 sum broken in trunk on sparc64=>auth md5 sum broken on big-endian machines (e.g., sparc64)
    Wed 15 Jun 2011 12:21:25 AM UTCjtnStatusNone=>In Progress
      Assigned toNone=>jtn
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup