What are we talking about here, just an addition to the GCheckSumType enumeration ?
Or something else ?

It could be implemented as another GCheckSumType if that's what's desired.

But in reality it has a slightly different use pattern than that of a checksum. In particular you provide a set of bytes as a key (and key length) to the constructor.

So I've been implementing it as another set of functions: g_hmac_xxx. I'll have something to look at shortly.

Created attachment 189892 [details] [review]
hmac: Implementation of HMAC in glib

This implements g_hmac_xxx() functionality using the standard checksum
functions supported by glib.

HMAC is a solid secure way to hash a key and a password. Many other
approaches fraught with append and prepend issues.

BTW, another patch coming with test cases...

Review of attachment 189892 [details] [review]:

the glib.symbols changes are missing.

::: glib/ghmac.c
@@ +92,3 @@
+
+  if (key_len < 0)
+    key_len = strlen ((char*)key);

we don't do this for binary blobs (const guchar*). if you're accepting any binary blob then you have to assume that it might include NUL bytes, and the length has to be >0.

@@ +294,3 @@
+gchar *
+g_compute_hmac_for_data (GChecksumType  digest_type,
+                         guchar        *key,

shouldn't this const guchar*?

@@ +295,3 @@
+g_compute_hmac_for_data (GChecksumType  digest_type,
+                         guchar        *key,
+                         gssize         key_len,

this should be gsize.

@@ +334,3 @@
+gchar *
+g_compute_hmac_for_string (GChecksumType  digest_type,
+                           guchar        *key,

same as above: shouldn't this be const guchar*?

@@ +335,3 @@
+g_compute_hmac_for_string (GChecksumType  digest_type,
+                           guchar        *key,
+                           gssize         key_len,

same as above: this should be gsize.

Review of attachment 189892 [details] [review]:

At a high level I'm uncertain about adding "hard" crypto to GLib.  For RHEL at least we're trying to standardize on NSS ( http://www.mozilla.org/projects/security/pki/nss/ ).  Yes, NSS has lots to hate about it (really it'd be cool if someone tried stubbing out NSPR and backing it by GLib), but it has been certified and proven.

Obviously checksums make sense in GLib since they have such a wide variety of uses, but HMAC is much more specialized.  I'm not saying "no", but I'd like to just raise the point.

Also, I wouldn't take this patch without tests.  Please test cases like key_len > digest_len etc.

::: glib/ghmac.c
@@ +42,3 @@
+ * Simple mechanisms for using SHA1 and other algorithms to digest a key and
+ * data together are vulnerable to various security issues. HMAC uses algorithms
+ * like SHA1 in a solid and secure way to produce a digest of a key and data.

Drop "in a solid", it's not adding anything.

@@ +44,3 @@
+ * like SHA1 in a solid and secure way to produce a digest of a key and data.
+ *
+ * Both the key and data are simple strings of bytes or characters.

"are arbitrary byte arrays".

@@ +60,3 @@
+ * @digest_type: the desired type of digest
+ * @key: the key for the HMAC
+ * @key_len: the length of the keys

For all of these, please add the introspection hints like:

@key: (array length=key_len)

@@ +71,3 @@
+ * using g_hmac_get_string(), which will return the checksum as a
+ * hexadecimal string; or g_hmac_get_digest(), which will return a
+ * vector of raw bytes. Once either g_hmac_get_string() or

I prefer the term "array" over "vector".

@@ +97,3 @@
+  hmac->digest_type = digest_type;
+  hmac->digest1 = g_checksum_new (digest_type);
+  g_return_val_if_fail (hmac->digest1, NULL);

Let's avoid g_return_if_fail in the middle of functions.  Better not to leak.  So you'd do:

GChecksum *checksum;

checksum = g_checksum_new (digest_type);

g_return_val_if_fail (checksum != NULL, NULL);

hmac = new()
hmac->digest1 = checksum
hmac->digest2 =  g_checksum_new (digest_type);  /* Note we assume this one can't fail, since the other one didn't */

::: glib/ghmac.h
@@ +36,3 @@
+ * An opaque structure representing a checksumming operation.
+ * To create a new GChecksum, use g_checksum_new(). To free
+ * a GChecksum, use g_checksum_free().

Mentions GChecksum not GHmac.

Review of attachment 189892 [details] [review]:

I share Colins concerns (and those of our security team, e.g tmraz) about spreading uncertified implementations of 'hard' crypto around. We have accepted the sha implementations here in glib with the argument that it is better to have a single implementation for all of the desktop (that we can then patch and replace with nss if desired) rather than having multiple spread through various modules. But do you think there same argument holds in this case ? What users of this do you see in the desktop ?

I've thought about this more today.

To be honest, I share your concerns. Initially I thought HMAC would be out of place in Glib. But I'm torn between those and the other concerns below.

In general, developers seem to shy away from complex crypto libraries. Sometimes this is justifiable (due to complex init of such libraries) and sometimes its not particularly justifiable. So developers end up rolling their own stuff. This is bad. I'm against roll your own crypto.

 * People are rolling their own HMAC in all sorts of places. Wocky, folks, and
   I'm sure other places I haven't looked.
 * People are rolling their own SHA(key + message) type stuff. This is really
   vulnerable to all sorts of attacks.

For those two reasons, I think the 'pros' of having GHmac in Glib, are greater than the 'cons'. We end up 'rolling our own' in Glib, so that everyone else doesn't do it with varying degrees of duplication and fail.

Created attachment 189990 [details] [review]
Updated with review changes and tests

Available as a branch here: http://cgit.collabora.com/git/user/stefw/glib.git/log/?h=hmac

More thoughts after some discussion...

BTW, HMAC is very simple, and should probably be considered in the same 'class' of crypto as SHA, MD5 and other hash algorithms. I can understand why some people would consider it 'hard' crypto. I think it's far less hard :) than the other stuff.

The entire security community has been chanting "use HMAC". I guess the risk is that if we make HMAC less accessible, people won't use it, and invent their own stuff.

Here's a compromise proposal: 

I'd be a lot more happy with accepting hmac api if it came with an --enable-nss switch that would use the nss implementation under the hood, optionally.

We'd need to look into at least the following. Just off the top of my head:

 * Wouldn't we need to initialize NSS in such a way that the databases aren't loaded.
 * Would this prevent other libraries from using NSS *with* the databases?
 * Look into thread safety issues. This Glib implementation is completely thread safe per GHmac object.
 * When do we run NSS shutdown?

Is there someone who wants to help with this research? 

Also, in my opinion, NSS dependent stuff belongs in glib-networking. But it seems kind of overkill to have a GIO hook for each of these things...

(In reply to comment #11)
> Here's a compromise proposal: 
> 
> I'd be a lot more happy with accepting hmac api if it came with an --enable-nss
> switch that would use the nss implementation under the hood, optionally.

glib linking to NSS and NSPR would be pretty ugly though =(   And honestly I think step 1) of anything like this is to shim out NSPR somehow.

Hmm.  So which modules would be potential users of GHmac?  You mentioned libfolks; any others?

(In reply to comment #13)
> Hmm.  So which modules would be potential users of GHmac?  You mentioned
> libfolks; any others?

(Actually, I think Stef meant libgdata; libfolks doesn't use HMAC at all, but libgdata uses it for OAuth.)

More generally, other applications implementing OAuth and friends need HMACs.

(In reply to comment #13)
> Hmm.  So which modules would be potential users of GHmac?  You mentioned
> libfolks; any others?

Ones that I know of: libgdata (as Phillip said), wocky, librest.

My gut feeling is that there's more, especially if we count reinventing the wheel with other questionable key+message digests.

So practically speaking crypto in GNOME right now appears to mostly be libgcrypt and gnutls (depended on by gnome-keyring and epiphany).  Neither implement HMAC.  NSS does however.

Given that state, I guess this argues somewhat for adding HMAC to GLib, given that nothing in GNOME right now deps on NSS.

(In reply to comment #12)
> We'd need to look into at least the following. Just off the top of my head:
> 
>  * Wouldn't we need to initialize NSS in such a way that the databases aren't
> loaded.
>  * Would this prevent other libraries from using NSS *with* the databases?

This issue got fixed at some point. (The Fedora Crypto Consolidation project couldn't get very far until it did.)

>  * Look into thread safety issues. This Glib implementation is completely
> thread safe per GHmac object.

NSS is thread-safe.

>  * When do we run NSS shutdown?

You don't. NSS_Shutdown() is just "free memory so that valgrind won't complain it's still in use". You don't have to call it if you don't care about that, and you are actively discouraged from calling it from libraries. (Any docs that claim otherwise are out of date.)

Review of attachment 189990 [details] [review]:

Ok, I think general consensus was to add this; if we want to e.g. later backend things with NSS, I think that'd be possible.

I haven't looked at correctness of the implementation in detail, but I did verify some of your test vectors, so that adds a lot of assurance.

::: glib/ghmac.c
@@ +115,3 @@
+
+  buffer = g_malloc (block_size);
+  pad = g_malloc (block_size);

Very minor; you could just g_alloca() these and avoid hitting malloc which adds fragmentation.

@@ +190,3 @@
+{
+  if (G_LIKELY (hmac))
+    {

Can we make this one a refcounted structure?  We've changed most GLib types at this point.

So it'd be g_hmac_unref() and g_hmac_ref().

@@ +200,3 @@
+ * g_hmac_update:
+ * @hmac: a #GHmac
+ * @data: buffer used to compute the checksum

This one also needs (array length=length)

Created attachment 190570 [details] [review]
Updated patch, refcounted, g_alloca

Thanks. Made those changes.

Branch is here: http://cgit.collabora.com/git/user/stefw/glib.git/log/?h=hmac

Should I squash this while merging, or just merge normally?

(In reply to comment #19)
> Created an attachment (id=190570) [details] [review]
> Updated patch, refcounted, g_alloca
> 
> Thanks. Made those changes.
> 
> Branch is here: http://cgit.collabora.com/git/user/stefw/glib.git/log/?h=hmac
> 
> Should I squash this while merging, or just merge normally?

Please squash and commit.  In GNOME generally we use bugzilla for detailed ugly history (in comparison to linux-kernel, it takes the place of the mailing list).

(In reply to comment #18)
> Review of attachment 189990 [details] [review]:
> 
> Ok, I think general consensus was to add this; if we want to e.g. later backend
> things with NSS, I think that'd be possible.

I actually want to see proof that it'll be possible; preferably in the form of a patch.

So please, don't commit this yet.

(In reply to comment #21)
> (In reply to comment #18)
> > Review of attachment 189990 [details] [review] [details]:
> > 
> > Ok, I think general consensus was to add this; if we want to e.g. later backend
> > things with NSS, I think that'd be possible.
> 
> I actually want to see proof that it'll be possible; preferably in the form of
> a patch.

Hmm...what do we suspect would be hard?  Basically it'd be:

* Add --enable-nss-crypto configure option
* For each file that adds crypto like gchecksum.c, add a new file gchecksum-nss.c which contains the same symbols, but implemented with NSS.  Documentation remains in gchecksum.c.
* AM_CONDITIONAL for the above

Maybe some details around a private gnss.c which does one-time initialization of NSS inside a g_once() etc.

Sounds like providing that patch should not be a big hurdle then...

Sounds like an interesting task for someone concerned with NSS.

Created attachment 190620 [details] [review]
patch to use NSS instead

ta da

Cool. I'll be away on vacation for a while, so I won't be responding for a bit.

But in any case, if the maintainers of glib would like to make a dependency on NSS for the sake of HMAC digests, then I won't complain.

I don't necessarily like this approach, but there's already enough stop energy when it comes to anything related to crypto (in some cases for good reason). So if using NSS is what it takes to solve this, then please go ahead and use Dan's patch.

(In reply to comment #27)
> Cool. I'll be away on vacation for a while, so I won't be responding for a bit.
> 
> But in any case, if the maintainers of glib would like to make a dependency on
> NSS for the sake of HMAC digests, then I won't complain.

Soo...if we were to take a dep on NSS, I think we should at least be semi clever and only pull it in if the application calls any crypto functions.

Also, I can easily imagine that some embedders might want to make NSS optional (note Yocto doesn't include NSS right now, only OpenSSL).  That argues for leaving in Stef's code as the --disable-nss implementation.

I read Matthias's comment as talking about future possibilities, not adding an NSS dep to glib now.

Thanks for the patch, Dan !

Yeah, I don't want a hard nss dep in glib now (or ever, really). 

I was envisioning a --enable-nss compile-time option that would cause a ghmac-nss.c to be compiled instead of ghmac.c. 

Once we got that, we should also look at replacing the various gchecksum implementations with nss ones.

*** Bug 587707 has been marked as a duplicate of this bug. ***

Oh, neat.  As mentioned earlier, librest has roll-your-own HMAC support.

BTW, python includes hash functions and hmac in the core distribution. 

Obviously we don't need to include everything that python does, but this helps the case for hmac not being 'hard' crypto, but rather a useful primitive for use in web authentication.

I'd like to go ahead and merge this as is. However if we're going to include the NSS bits right now (as opposed to later) then someone else should probably take this bug over.

Sure, I'm ok with merging this now.

Merged into master.

Fabrizio, sorry that I didn't see your implementation before writing this. I really should have checked whether someone had already implemented a patch.