[PATCH 3/3] Implement OAP for SGSN registration.

[Holger Freyther]

> 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