On 29 Sep 2015, at 15:10, Neels Hofmeyr
<nhofmeyr(a)sysmocom.de> wrote:
/me replying to two mails in one
In summary, the questions I'd like answered, see below for details:
- using memcmp() is ok? (re constant_time_cmp())
When comparing the result we should use a constant time compare as otherwise
the attacker can find out (by timing) how many bytes of the result were correct and
by this determine/find the secrets
The classic memcmp will stop on the first byte that is not the same.
yes k and opc from two diferent keys would be
great.
I can make it one double-length shared secret and use one half each?
Ah whatever, I'll add secret_k and secret_opc, who knows if we'd like to have
an operator specifc key one day…
thanks
+struct
gprs_ipa_client {
+ struct ipa_client *ipac;
In general we subclass by embedding the parent class directly because then
Problem being, we get a pointer from the constructor:
struct ipa_client *ipa_client_create(const char *ip_addr, [...]
(was gsup_client_create(), i.e. someone else wrote this.)
Should I refactor this to
void ipa_client_create(struct ipa_client*, ...)
?? I'm trying to limit API refactoring efforts though.
you are right, then leave it as it is.
Not sure what you mean by "split with message
encoding/decoding", like,
commit the oap_messages.c in a separate commit? IMHO it's already
separate.
but I review top to bottom and then stop somewhere in the middle. if you
have separate patches I at least have a natural break somewhere and can
easily resume it. :)
log an error at least.
Am not clear yet how an error in a protocol callback should propagate to
the main loop and barf there. Certainly something grave should happen.
“log an error” means LOGP(BLA, LOGL_ERROR, “Something bad happened %d\n”, rc);
+ switch (oap_msg.message_type) {
+ case GPRS_OAP_MSGT_CHALLENGE_REQUEST:
don’t do this in-place. Please call a function
Don't do what in-place? You mean factor the guts of "case: { ... } break;"
out
to a new function?
Yes, factor out the significant code between the {}.
>>> - if (hh->proto != IPAC_PROTO_OSMO)
>>> - goto invalid;
>>> -
>>> - if (!he || msgb_l2len(msg) < sizeof(*he) ||
>>> - he->proto != IPAC_PROTO_EXT_GSUP)
>>> + if (!he || msgb_l2len(msg) < sizeof(*he))
>>> goto invalid;
>
>> I think you want to have the first
hh->proto check here. As the second part will
>> use he which is only valid/there if it is IPAC_PROTO_OSMO?
> Hmmm. Is it? I'd love to assumed that
there is always both a PROTO and
> PROTO_EXT value available... ipaccess_head_ext is defined in ipaccess.h, so is
> it really specific to IPAC_PROTO_OSMO?
If there is a PROTO_EXT there is a PROTO but a PROTO does not imply the
PROTO_EXT.
hrm. It's so much more effort :P
I often disagree on how people use commenting styles...
anyway, I'll submit to local rule. Is it: *never* use '//‘ ?
Only if the Linux Kernel starts using //
>> * “ipa” is such a vague name. The SGSN
might have several IPA connections to
>> different kind of systems. One might be used for GSUP. Another one might be for
>> sending telemetric data. We use this connection for GSUP so let’s keep the
current
>> configuration settings.
> Was not aware of it, my name choice based
on the naive assumption that there is
> just the one IPA connection.
> But we're also talking OAP on that IPA
line.
> Can I call it gprs_map_proxy_client, plz?
Check with Jacob, he had a reason to pick gsup. For me both names are fine (and
gsup slightly favoarable as it is less long).
>>> -/* Wishful thinking to generate a constant time compare */
>>> -static int constant_time_cmp(const uint8_t *exp, const uint8_t *rel, const
int count)
>
>> ah ha, but then please don’t call it
gprs_constant_time..
> wasn't me, was it. I just copied it
around. I guess I'll use memcmp() and leave
> constant_time_cmp() where it belongs: a dark corner of a static C file context.
you probably want a constant time comparison too.
cheers
holger