This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/OpenBSC@lists.osmocom.org/.
Holger Freyther holger at freyther.de> On 29 Sep 2015, at 15:10, Neels Hofmeyr <nhofmeyr at 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