2.x.x (and earier?): yet another decoding SSHA issue

[Stefan Winter]

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 at 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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freeradius.org/pipermail/freeradius-devel/attachments/20130716/948f707a/attachment.pgp>