Re: [Linphone-developers] bzrtp support for AES with 256-bit keys

[Ben Sartor]

Hi Johan,

thanks for merging.

Here are some more patches which should implement your suggestion. The first 
ones apply to mediastreamer2 and the second ones to linphone.

Cheers,
 Ben

> Hi Ben,
> thanks. It seems all fine. I'll push it soon.
> 
> I won't be able to work on liblinphone/mediastreamer2 before mid-february.
> The idea would be to use the already existing SRTP setting in the
> .linphonerc config file to set the cipher block algo preference and add
> a field in this file to select key agreement algo(to be able to disable
> non mandatory DH2k).
> 
> regards,
> 
> johan
> 
> On 27/01/15 05:30, Ben Sartor wrote:
> > Hi Johan,
> > 
> > thanks for reviewing.
> > 
> >> Hi Ben,
> >> and thanks for the patch.
> >> 
> >> I had a quick look at it, very nice rework on the tests cases!
> >> 
> >> Few remarks:
> >> - I think we should stick to AES128 being the default (switch in
> >> bzrtpCrypto_getAvailableCryptoTypes). The user will set it to AES256 if
> >> needed.
> > 
> > Agreed, especially as it is possible to update bzrtp without updating
> > mediastream2 this way.  Updated patches are attached.
> > 
> >   By the way, this is about bzrtp and (lib)linphone may still choose
> >   AES256 as> 
> > default in the future.
> > 
> >> - You may want to export also the algorithm defines and not only the
> >> types in bzrtp.h (commit 4) otherwise the user won't be able to safely
> >> use the get/set functions.
> > 
> > Done.
> > 
> >> - We shall ensure that mandatory algorithms are always present in the
> >> context supported list, by either checking their presence in the input
> >> supportedTypes of bzrtp_setSupportedCryptoTypes and queing them at the
> >> end if they are missing or tweaking the selectCommonAlgo to add them if
> >> they are missing.(first one seems cleaner)
> >> 
> >> - Same kind of problem (which was already there before your patches)
> >> with the incoming HelloPacket parsing: we shall ensure that if the
> >> packet doesn't contain a mandatory algo, it is added at the end of the
> >> list as specified in the RFC section 5.2:
> >> "If a mandatory algorithm is not included in the list, it
> >> 
> >>      is implicitly added to the end of the list for preference."
> >> 
> >> Which means the in function bzrtp_packetParser, all the tests on
> >> messageData->xx == 0 to set the mandatory algo shall be replaced by
> >> something checking the presence of the mandatory algo in the
> >> messageData->xx field after parsing it from the packet.
> > 
> > Patches implementing this are attached.
> > 
> > Cheers,
> > 
> >   Ben
> >> 
> >> - min function seems fine to me as it is.
> >> 
> >> I'll have to rework the mediastream patch as I've done some
> >> modifications in zrtp.c, they're in branch dev_dtls for now but will be
> >> merge to master soon.
> >> 
> >> regards,
> >> 
> >> johan
> >> 
> >> On 23/01/15 03:58, Ben Sartor wrote:
> >>> Hi Johan,
> >>> 
> >>> thanks for you answer. I have tried to implement things like you
> >>> suggested.
> >>> 
> >>> Patches 0001 and 0002 are unchanged.
> >>> 
> >>> Patches 0003 to 0005 add the the getter and setter you suggested.
> >>> 
> >>> Patches 0006 to 0009 implement the tests. I completely refactored the
> >>> function "test_algoAgreement". Is it ok for you to write tests this way?
> >>> 
> >>> Patches 0010 to 0012 are some code cleanup suggestions and were not
> >>> discussed before. Let me know what you think.
> >>> Is there a better place for the min-function? Maybe even a macro like
> >>> discussed here [1]? Is "__typeof__" ok?
> >>> 
> >>> [1] http://stackoverflow.com/questions/3437404/min-and-max-in-c
> >>> 
> >>> The mediastreamer2 patch of my initial post still applies and may be
> >>> used
> >>> for testing.
> >>> 
> >>> Kind Regards,
> >>> 
> >>>    Ben
> >>>> 
> >>>> Hi Ben,
> >>>> 
> >>>>> Yes. Just to be sure, did you mean implementing functions like this:
> >>>>> 
> >>>>> void bzrtp_setSupportedCipherTypes(bzrtpContext_t *zrtpContext,
> >>>>> uint8_t
> >>>>> availableTypes[7], const uint8_t availableTypesCount);
> >>>>> 
> >>>>> uint8_t bzrtp_getSupportedCipherTypes(bzrtpContext_t *zrtpContext,
> >>>>> uint8_t
> >>>>> availableTypes[7]);
> >>>> 
> >>>> Yes but you want to add an uint8_t algoType argument(just like
> >>>> bzrtpCrypto_getAvailableCryptoTypes function) to both of them in order
> >>>> to use the same function to get/set the available algos for block
> >>>> cipher/key exchange/SAS rendering/Hash.
> >>>> 
> >>>>>> This means we also must add a way to store the user configuration in
> >>>>>> linphone. I was thinking the easiest way would be to store it in the
> >>>>>> config file and access it only manually for now. I can implement this
> >>>>>> if
> >>>>>> you're lost on the way linphone manage the config file.
> >>>>> 
> >>>>> I haven't had a look to  linphone config file management, yet. Let's
> >>>>> see
> >>>>> how far I get or if you find time first.
> >>>> 
> >>>> It's quite simple, but if you struggle tell me and I'll have a look at
> >>>> it when you're done with bzrtp. We can use an already existing config
> >>>> setting used to select SRTP protection profile(see misc.c const
> >>>> MSCryptoSuite * linphone_core_get_srtp_crypto_suites(LinphoneCore
> >>>> *lc);)
> >>>> for the block cipher algo selection and do something for the other algo
> >>>> types when needed.
> >>>> 
> >>>>>> Last, this must be covered by automatic tests.(Key exchange between
> >>>>>> two
> >>>>>> users using different set of cipher block algo)
> >>>>> 
> >>>>> I'm not sure what you mean: Would you prefer a test similar to the
> >>>>> existing
> >>>>> "test_algoAgreement" or would it be better to write a test for the
> >>>>> function
> >>>>> "selectCommonAlgo" directly?
> >>>> 
> >>>> I was thinking of extending the test_algoAgreement to include block
> >>>> cipher selection and some test on linphone call to check correct
> >>>> reading
> >>>> of the configuration from file, but it can't harm to have a test for
> >>>> the
> >>>> selectCommonAlgo too.
> >>>> 
> >>>> Thanks for your contribution.
> >>>> 
> >>>> Have a good day
> >>>> 
> >>>> johan
> >>>> 
> >>>> _______________________________________________
> >>>> Linphone-developers mailing list
> >>>> address@hidden
> >>>> https://lists.nongnu.org/mailman/listinfo/linphone-developers
> >>> 
> >>> _______________________________________________
> >>> Linphone-developers mailing list
> >>> address@hidden
> >>> https://lists.nongnu.org/mailman/listinfo/linphone-developers
> >> 
> >> _______________________________________________
> >> Linphone-developers mailing list
> >> address@hidden
> >> https://lists.nongnu.org/mailman/listinfo/linphone-developers
> > 
> > _______________________________________________
> > Linphone-developers mailing list
> > address@hidden
> > https://lists.nongnu.org/mailman/listinfo/linphone-developers
> 
> _______________________________________________
> Linphone-developers mailing list
> address@hidden
> https://lists.nongnu.org/mailman/listinfo/linphone-developers

-- 
———————————————————
https://www.simlar.org
free and secure calls

fon: +49-(0)221-999 999 30
fax: +49-(0)221-999 999 31                                   
mail: address@hidden

github: https://github.com/simlar/
———————————————————

0001-added-zrtp-support-for-AES-with-256-bit-keys-AES3.patch
Description: Text Data

0002-enhanced-zrtp-parameters-by-crypto-types.patch
Description: Text Data

0003-added-to_string-and-from_string-for-enums-MSZrtpHash.patch
Description: Text Data

0001-configfile-now-supports-setting-zrtps-cipher-and-aut.patch
Description: Text Data

0002-zrtp-now-prefers-AES256-and-HS80.patch
Description: Text Data

0003-configfile-now-supports-setting-zrtps-key-agreements.patch
Description: Text Data