I've changed check_nonce() to:
.....
if (nonce->len < MIN_NONCE_LEN) {
return 1;
}
since = get_nonce_since(nonce);
if (since < up_since) {
return 3;
}
expires = get_nonce_expires(nonce);
if (calc_nonce(non, &non_len, cfg, since, expires, secret1,
.....
It causes that nonce from previous ser instance is always considered as
stalled even it's not necessary in case when secrets are not generated,
i.e. remain the same. I don't think it worth testing memcmp first and
only if differs test since<up_since. I think it's marginal case and
extra stale response is not problem. Do you agree?
If yes I'll commit it.
Tomas
Andrei Pelinescu-Onciul napsal(a):
> On Jul 02, 2008 at 12:19, Tomas Mandys <[EMAIL PROTECTED]> wrote:
>> Andrei Pelinescu-Onciul napsal(a):
>>> Hi,
>>>
>>> I've just finished changing auth to use base64 nonces. However I did find
>>> a few strange things that I want to check with you before commiting:
>>>
>>> 1. in check_nonce(...) in the beginning we have something like:
>>> if (get_nonce_len(cfg) != nonce->len) {
>>> return 1; /* Lengths must be equal */
>>> }
>> If you need consider changing extra checks then probably yes, but if you
>> omit this check then function never returns "1". But
>> get_auth_checks(msg) return value is not transparent for me.
>
> Yes, right, there should be some form of it.
> I think:
> if (nonce->len<MIN_NONCE_LEN){
> return 1; /* length musth be >= then minimum length */
> }
>
> is enough. It catches nonces which are too small.
> The get_auth_checks(msg) just tells if another MD5, appended to the end
> of the nonce should be checked.
> If we have a ser without auth_extra_checks restarted with
> auth_extra_checks, then IMHO the new ser should consider a old nonce as
> stale (and this would happen, the old nonce should be catched below by the
> if (since < up_since)).
> The only problems with this code might be that if auth_extra_checks are
> turned off a longer nonce might be accepted, if it's built like :
> valid_nonce_value + extra_stuff. But I don't think it is a problem if
> somebody decides to add crap to a nonce (the crap part will be ignored
> if the "required" part is ok).
>
>>> I think this should be deleted, because one can have for example a ser
>>> restart with different cfg (e.g.: auth_extra_checks enabled) which might
>>> change the nonce length. So having a different nonce lenght from the
>>> configured one, is a valid case that should be caught by the new
>>> up_since check.
>>>
>>> 2. check_nonce(): the if (since < up_since) check is inside an if that
>>> get executed only if calc_nonce(...) fails. However calc_nonce(..) fails
>>> only if passed a nonce buffer which is too small to hold the entire
>>> nonce (which never happens in this case).
>>> I think it should be moved outside that if.
>> Oaps, yes it should be put before calc_nonce() function.
>>
>>> 3. is it ok if instead of calling is_nonce_stale() in post_auth, I call
>>> it in check_nonce() and mark it (auth->stale=1) in auth_check_hdr_md5?
>>> The problems is that now the nonce is a base64 encoded binary blob and I
>>> want to decode it only once in check_nonce() and avoid decoding it a
>>> second time in post_auth.
>>>
>>>
>
> Andrei
--
-----------------------------
Tomas Mandys
[EMAIL PROTECTED]
Tekelec Czech Republic s.r.o.
-----------------------------
_______________________________________________
Serdev mailing list
[email protected]
http://lists.iptel.org/mailman/listinfo/serdev
