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

[Arran Cudbard-Bell]

On 16 Jul 2013, at 11:04, Stefan Winter <stefan.winter at restena.lu> wrote:

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

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.

Accepted, this is a pretty odd case. I can't think of many other places
where attributes can contain values in multiple formats. Other than that
awful WiMAX combo IP attribute.

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

No.

> * Or encode the already base64-encoded SSHA hashes inside a hex encoding
> to make the heuristics happy?

Yes. Or convert the encoding to hex, either will work.

> * 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?

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.

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?

-Arran

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