On 16 Jul 2013, at 10:28, Arran Cudbard-Bell <a.cudbardb@freeradius.org> wrote:

On 16 Jul 2013, at 08:58, Stefan Winter <stefan.winter@restena.lu> wrote:

...

Hi,

here's yet another thing which caused spurious authentication failures; luckily I was accidently in debug mode looking for something else when I found the cause for some failed auth:

+++- entering policy redundant {...} [sql-imap-hash] expand: %{User-Name} -> pgloden [sql-imap-hash] sql_set_user escaped user --> 'pgloden' rlm_sql (sql-imap-hash): Reserving sql socket id: 7 [sql-imap-hash] expand: (SELECT id, username, attribute, value, op FROM check_imap_ssha1 WHERE username='%{SQL-User-Name}') -> (SELECT id, username, attribute, value, op FROM check_imap_ssha1 WHERE username='pgloden') rlm_sql_mysql: query: (SELECT id, username, attribute, value, op FROM check_imap_ssha1 WHERE username='pgloden') rlm_sql: Failed to create the pair: Non-hex characters at QX rlm_sql (sql-imap-hash): Error getting data from database [sql-imap-hash] SQL query error; rejecting user

The query works just fine and returns a base64-encoded SSHA1 hash of a user's password. The base64 blob happens to start with the two characters "0X" (and then continues with QX, which is in the error message).

It seems like the query response parser tries to be overly clever and thinks that anything starting with 0X is a hex number and needs decoding. This, of course, fails. Needsless to say that the schema doesn't suggest an integer at all, it's a varchar(168).

The workaround right now for us is to set the hashed password, see if it happens to start with 0X, and if so, try with a different salt.

At a guess, i'd say the attribute you were creating was of type OCTETS, in which case the above is the correct behaviour. Can you double check that the type of the attribute is string, and not octets.

Nearly all string to value conversions (all drivers produce values as strings) go through pairparsevalue. String attributes will *not* be treated as hex strings by pairparsevalue, only raw attributes are.

Looking at src/lib/valuepair.c (pairparsevalue), writing to a destination attribute of type octets would describe exactly when you're experiencing.

*What Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team

Hi,

At a guess, i'd say the attribute you were creating was of type OCTETS, in which case the above is the correct behaviour. Can you double check that the type of the attribute is string, and not octets.

It's SSHA1-Password. Yes, it is octets, as defined in dictionary.freeradius.internal.

Nearly all string to value conversions (all drivers produce values as strings) go through pairparsevalue. String attributes will *not* be treated as hex strings by pairparsevalue, only raw attributes are.

Looking at src/lib/valuepair.c (pairparsevalue), writing to a destination attribute of type octets would describe exactly when you're experiencing.

I certainly wouldn't call it "correct". All other values for SSHA1-Password which don't incidentally start with an 0X get parsed and used correctly. Looking at the code, it's much more so that lib/valuepair.c's case PW_TYPE_OCTETS does a (poor) heuristics on the first two bytes of the incoming blob. If it finds a 0x then it thinks it needs to decode. Otherwise, it just returns vp. What are you suggesting to make SSHA1-Password work deterministically? * Should I change the dictionary.freeradius.internal away from octets to string? * Or encode the already base64-encoded SSHA hashes inside a hex encoding to make the heuristics happy? * Or should the heuristics maybe become a little more clever to *try* to hex-decode things which start with 0x, but just return the value as-is if that failed? Greetings, Stefan Winter -- Stefan WINTER Ingenieur de Recherche Fondation RESTENA - Réseau Téléinformatique de l'Education Nationale et de la Recherche 6, rue Richard Coudenhove-Kalergi L-1359 Luxembourg Tel: +352 424409 1 Fax: +352 422473

Would this solve the issue to your satisfaction for 3.0.0 at least, or do see it as more magical unexpected behaviour?

Please no magical behaviour. Can't the cast/reference stuff be used to do this "properly"?

Hi,

It's a binary attribute, it's expected. There are many places where I disagree with how the server operates, but assigning a string value starting with 0x to a binary attribute and it erroring out when it finds non-hexits is fine IMHO.

I'm fine with the heuristics itself; but it should be more self-aware that can get into false positives, and be more gentle if it finds itself on the wrong track. So, if there is a 0x in the beginning, yes, it's reasonable to assume that it should be decoding; but there are cases where it's the wrong thing to do. Throwing an error is pretty harsh then; given that the same parser is otherwise very forgiving and lets random junk pass by without further fuss. I also think that changing the errors to a radlog, but return vp, does not break anything on existing, working deployments, but enables more cases to work. It is also very gentle on the source; looking at lib/valuepair.c, four lines of code change would do wonders: For the odd-number-of-characters around line 1100: if ((strlen(cp) & 0x01) != 0) { radlog(L_INFO, "The input superficially looks like it's hex-encoded, but it's not. Interpreting as literal octet stream."); break; } For the 0x prefix immediately after: if (sscanf(cp, "%02x", &tmp) != 1) { radlog(L_INFO, "The input superfially looks like it's hex-encoded, but it's not (character sequence %c%c is not in the hexadecimal alphabet). Interpreting as literal octet stream.", cp[0], cp[1]); break; } That solves the problem as good as it gets: the remaining case is that an input is meant literal, has an even number of characters *and* all these characters happen to beinside the 0-9a-f range. That case can't be solved without enforcing strict typing of the input; the phenotypes "hex-encoded string" and "literal value" simply are not distinguishable without meta-data if they use their common subset of chars. The above code basically transfers the previous "I guessed wrong, and you have the trouble." to a "I guessed wrong, so I'm backing off, maybe you know what you are doing." I find that attitude much more appropriate. It doesn't solve things deterministically, and I wouldn't want it as a new approach, but for a 2.2.x line which will not allow for bigger changes, it would look like "good enough" for me.

We could do that, but the majority of cases where you're assigning a string value to an attribute of type of octets and the value starts with 0x you do want to do a hex to binary value conversion.

Do it, but be gentle if your doing was wrong. :-)

My strong preference in this case is to update pairparsevalue to automatically convert base64 strings to binary data. I believe it's possible to do pretty accurate heuristics on the string to determine it's in base64 encoding. This would solve this issue, and potential issues with any other password hashes.

I'm not comfortable making that change on 2.x.x without Alan's input. But I cannot seeing it cause major issues for 3.0.1 and it would avoid these sorts of issues in future.

Would this solve the issue to your satisfaction for 3.0.0 at least, or do see it as more magical unexpected behaviour?

Any heuristics is bad. This particular one is not documented anywhere; you have to read the source code to see what's going on/going wrong. If the intent is that all input to the octets type is to be hex-encoded, then it would be much better to enforce that and bail out if anything but a 0x... comes in. I.e.: do input validation before input interpretation. That creates hard failures which are seen immediately and can be taken care of before a production rollout. If this happens silently during runtime, and only for a small fraction of users, then it's likely to be detected after things are up and running in production; that's our case. The problem with your proposed base64 decoding is again the same as this one: there will be a heuristic at work, and if some poor soul happens to assign a value with is not base64, but looks undistinguishably like it, then he'll run into a really unexpected error that needs a lot of digging in code to even understand. Your heuristics could be "pretty accurate" indeed; but it's still possible that it fails. All you need is a raw octet value which by chance consists of only the allowed characters of the base64 alphabet. This is not impossible. This simply does not scale well with the number of users/attributes to manage. We have approx 16.000 users and as many SSHA1 hashes - and had roundabout 15 users being hit by the 0x prefix issue. Maybe your proposed base64 one is better - but then, if an ISP has a million users and "a few hundred" can't authenticate due to the heuristics going wrong then that's still bad. Greetings, Stefan Winter

-Arran

Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team

- List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html

-- Stefan WINTER Ingenieur de Recherche Fondation RESTENA - Réseau Téléinformatique de l'Education Nationale et de la Recherche 6, rue Richard Coudenhove-Kalergi L-1359 Luxembourg Tel: +352 424409 1 Fax: +352 422473

On 16 Jul 2013, at 14:23, Arran Cudbard-Bell <a.cudbardb@freeradius.org> wrote:

...

It is also very gentle on the source; looking at lib/valuepair.c, four lines of code change would do wonders:

For the odd-number-of-characters around line 1100:

if ((strlen(cp) & 0x01) != 0) { radlog(L_INFO, "The input superficially looks like it's hex-encoded, but it's not. Interpreting as literal octet stream."); break; }

For the 0x prefix immediately after:

if (sscanf(cp, "%02x", &tmp) != 1) { radlog(L_INFO, "The input superfially looks like it's hex-encoded, but it's not (character sequence %c%c is not in the hexadecimal alphabet). Interpreting as literal octet stream.", cp[0], cp[1]); break; }

That solves the problem as good as it gets

Or you can define a local string attribute which you write the value to, and use the base64 xlats to correctly

I agree with your points, but it's a non-trivial behaviour change. Thinking about it more, although it's very unlikely hex and plaintext could get conflated with base64, it's still possible, and adding more implicit conversions makes it worse.

I'll let Alan decide about the firmness of hex decoding, but there is an easier solution for you in 2.2.1.

Define a local string type attribute to hold the base64 encoded value.

update control { SSHA1-Password = "%{base64tohex:SSHA1-Password-Base64}" }

SSHA1-Password will then hold the raw octet value of the hash. Unfortunately I believe that rlm_pap has it's own normalization logic, so may still attempt to decode the raw octets as hex or base64 *sigh*.

actually, no, because rlm_pap knows the expected raw length of the hash it can be absolutely certain about which encoding scheme it's in. The above is an immediate solution to the problem without requiring code changes. Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team

Hi,

...

SSHA1-Password will then hold the raw octet value of the hash. Unfortunately I believe that rlm_pap has it's own normalization logic, so may still attempt to decode the raw octets as hex or base64 *sigh*.

Only if the data is longer than the length of the binary hash.

i.e.

- length == length of hash ---> DONE

- length is 4/3 (or so) + other stuff.. --> base64

- starts with "0x" and length is 2x the length of the hash --> hex

It should be pretty fail-safe.

Really? This is *salted* SHA; and the salt gets prepended (or appended) to the hash value. Since the length of the salt is arbitrary, the length of the resulting attribute value is also arbitrary (well, lower bound is the size of the unsalted hash of course). Even the 4/3 rule breaks at that point... Stefan -- Stefan WINTER Ingenieur de Recherche Fondation RESTENA - Réseau Téléinformatique de l'Education Nationale et de la Recherche 6, rue Richard Coudenhove-Kalergi L-1359 Luxembourg Tel: +352 424409 1 Fax: +352 422473

On 16 Jul 2013, at 15:54, John Dennis <jdennis@redhat.com> wrote:

On 07/16/2013 09:40 AM, Alan DeKok wrote:

...

Arran Cudbard-Bell wrote:

...

SSHA1-Password will then hold the raw octet value of the hash. Unfortunately I believe that rlm_pap has it's own normalization logic, so may still attempt to decode the raw octets as hex or base64 *sigh*.

Only if the data is longer than the length of the binary hash.

i.e.

- length == length of hash ---> DONE

- length is 4/3 (or so) + other stuff.. --> base64

- starts with "0x" and length is 2x the length of the hash --> hex

It should be pretty fail-safe.

I'm not a fan of heuristics, every time you think you've got the logic nailed down some corner case will prove you wrong. Plus it's magical, I don't like magic, I prefer explicit well defined behavior. Plus in this instance the behavior is tied to well known digests, what about all the other places where binary conversion from text encoding might be called for? Shouldn't everything obey the same rules?

Is it possible to add a qualifier indicating the format of the item, e.g. base64, hex, etc.?

You could use as part of the atribute name to indicate a cast. <string>SSHA-Password := <hash> But it's still awful. Anyway Stefan's point about SSHA is correct. Maybe an option to turn off the normalisation done by rlm_pap would be useful. Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team

On 16 Jul 2013, at 16:53, Matthew Newton <mcn4@LEICESTER.AC.UK> wrote:

On Tue, Jul 16, 2013 at 04:12:53PM +0100, Arran Cudbard-Bell wrote:

...

...

Is it possible to add a qualifier indicating the format of the item, e.g. base64, hex, etc.?

You could use as part of the atribute name to indicate a cast.

<string>SSHA-Password := <hash>

But it's still awful.

Anyway Stefan's point about SSHA is correct. Maybe an option to turn off the normalisation done by rlm_pap would be useful.

Having rehashed the rlm_pap code for v3, I'd question anyone's sanity wanting to touch the v2 code... I'm still in recovery :-)

Hehe. You should of seen the LDAP module, Alan gets major props for tackling that one :)

I've done a pull request for v3 that adds this option. It compiles, but I've not tested it yet.

Eh there's not much to go wrong.

The most sane thing for rlm_pap in v2 would be to pull in the module from master, but it probably wouldn't be hard to add a normalize option to that as well.

TBH it's extremely unlikely that if people use pre-decoded password hashes that there'll be an issue. Especially if they keep their salts to a sane length. It's just useful to be able to turn off normalisation where it's not required to completely eliminate the chance of any issues occurring. Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team

Or do I still have to change my encoding of the hashes from base64 to hex in the DB attribute's value, and *additionally* use the string cast later on in rlm_pap to prevent any further touching of the hash value?

In any case, let me know when there's something to test in 2.x.x.

The solution I posted earlier should work fine: update control { SSHA1-Password = "%{base64tohex:SSHA1-Password-Base64}" } You will need to define the local string attribute 'SSHA1-Password-Base64' and change the attribute in the database. The worry was normification could potentially mangle the binary version of the password further, hence the additional discussion around disabling it. Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team

Stefan Winter wrote:

* when using: SSHA1-Password = "%{base64tohex:RESTENA-SSHA1-Password}"

That won't work.

I tried to be clever and force an expansion of the attribute content with:

* SSHA1-Password := "%{base64tohex:%{RESTENA-SSHA1-Password}}"

That should work. So long as you put RESTENA-SSHA1-Password in the request list.

But that makes xlat fail completely. Maybe that's me not understanding unlang enough though.

rlm_sql_mysql: query: (SELECT id, username, 'RESTENA-SSHA1-Password', value, op FROM check_smtp_ssha1 WHERE username='xyz') [sql-smtp-hash] User found in radcheck table rlm_sql (sql-smtp-hash): Released sql socket id: 2 +++[sql-smtp-hash] returns ok ++- policy redundant returns ok expand: %{RESTENA-SSHA1-Password} ->

So RESTENA-SSHA1-Password doesn't work.

This is on 2.x.x from today's GIT BTW, as it's the first version that works without any modifications on systemd. Except for the 0X thing of course; otherwise I'd be happy with a 2.2.1 rollout.

We'll get that sorted out, and release 2.2.1. Alan DeKok.

Hi,

0x%{base64tohex: %{control:RESTENA-SSHA1-Password1}}

Sorry (again), but this still doesn't get the job done. xlat is now happy and produces a hex value, but it must be doing something wrong; I'm getting "Passwords don't match.". This is today's GIT of 2.x.x. Since this is an en-/decoding thing, I don't mind sending a temporary cleartext password along with the debug. I can confirm that the base64 hash itself is in order: if I send the incoming request to a different virtual server, which uses the DB's SSHA1-Password attribute directly, I can authenticate. Greetings, Stefan Winter # ./radtest swinter Exclamation\!Mark123 127.0.0.1:1812 123 testing123 Sending Access-Request of id 171 to 127.0.0.1 port 1812 User-Name = "swinter" User-Password = "Exclamation!Mark123" NAS-IP-Address = 158.64.2.206 NAS-Port = 123 Message-Authenticator = 0x00000000000000000000000000000000 rad_recv: Access-Reject packet from host 127.0.0.1 port 1812, id=171, length=20 And the relevant part of the debug output is: ++- entering policy redundant {...} [sql-smtp-hash] expand: %{User-Name} -> swinter [sql-smtp-hash] sql_set_user escaped user --> 'swinter' rlm_sql (sql-smtp-hash): Reserving sql socket id: 2 [sql-smtp-hash] expand: (SELECT id, username, 'RESTENA-SSHA1-Password', value, op FROM check_smtp_ssha1 WHERE username='%{SQL-User-Name}') -> (SELECT id, username, 'RESTENA-SSHA1-Password', value, op FROM check_smtp_ssha1 WHERE username='swinter') rlm_sql_mysql: query: (SELECT id, username, 'RESTENA-SSHA1-Password', value, op FROM check_smtp_ssha1 WHERE username='swinter') [sql-smtp-hash] User found in radcheck table rlm_sql (sql-smtp-hash): Released sql socket id: 2 +++[sql-smtp-hash] returns ok ++- policy redundant returns ok expand: %{control:RESTENA-SSHA1-Password} -> oPQYKSRg5w8XWEiJCcNtzKRhUhtJMUQ/WjdCWlVQS2JWN2Qz expand: 0x%{base64tohex: %{control:RESTENA-SSHA1-Password}} -> 0xffff18292460ff0f175848ff09ff6dffff61521b4931443f5a37425a55504b6256376433 ++[control] returns ok ++[pap] returns updated Found Auth-Type = PAP # Executing group from file /usr/local/freeradius/config/raddb/sites-enabled/SMTP +- entering group PAP {...} ++- entering policy pap_hash_debugfallback {...} +++- entering group {...} [pap] login attempt with password "Exclamation!Mark123" [pap] Using SSHA encryption. [pap] Passwords don't match ++++[pap] returns reject ++++? if ("%{control:RESTENA-Debug-Password}" ) expand: %{control:RESTENA-Debug-Password} -> ? Evaluating ("%{control:RESTENA-Debug-Password}" ) -> FALSE ++++? if ("%{control:RESTENA-Debug-Password}" ) -> FALSE [pap] login attempt with password "Exclamation!Mark123" [pap] Using SSHA encryption. [pap] Passwords don't match ++++[pap] returns reject +++- group returns reject ++- policy pap_hash_debugfallback returns reject Failed to authenticate the user. } # server SMTP -- Stefan WINTER Ingenieur de Recherche Fondation RESTENA - Réseau Téléinformatique de l'Education Nationale et de la Recherche 6, rue Richard Coudenhove-Kalergi L-1359 Luxembourg Tel: +352 424409 1 Fax: +352 422473

On 22 Jul 2013, at 10:43, Stefan Winter <stefan.winter@restena.lu> wrote:

Hi,

...

expand: %{control:RESTENA-SSHA1-Password} -> oPQYKSRg5w8XWEiJCcNtzKRhUhtJMUQ/WjdCWlVQS2JWN2Qz expand: 0x%{base64tohex: %{control:RESTENA-SSHA1-Password}} -> 0xffff18292460ff0f175848ff09ff6dffff61521b4931443f5a37425a55504b6256376433

when I take this base64 and use an online base64-decode service such as http://base64decode.net/ and use its output with an online hex-encoder such as http://convertstring.com/EncodeDecode/HexEncode I end up with the string

203F182924603F0F1758483F093F6D3F3F61521B4931443F5A37425A55504B6256376433

The bit-diff to what the FreeRADIUS fucntion produces is like:

203F182924603F0F1758483F093F6D3F3F61521B4931443F5A37425A55504B6256376433 ffff18292460ff0f175848ff09ff6dffff61521b4931443f5a37425a55504b6256376433 ***---------*---------*---*---*-*---------------------------------------

(I was wondering from the start why there are many more FF's in the FreeRADIUS version than what probability would suggest)

Yes that was the first thing I did. I suspected the base64 decoder (it's new), but, I just tested with rlm_pap in version 3.0 which uses that decoder and it works fine. The code is pretty simple... I'll keep digging. Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team

Arran Cudbard-Bell wrote:

sprintf "%02x" was giving the wrong result. I'm not sure why.

Likely signed / unsigned issues. <sigh>

I've switched to our own internal hex encoder which is probably faster anyway, and it gives the correct hex encoding.

That's better. Alan DeKok.