On 29 Sep 2015, at 15:10, Neels Hofmeyr nhofmeyr@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