HMAC signing for http push #5

redmoo · 2017-07-20T20:51:25Z

This is part of my contribution to Google Summer of Code 2017.

I implemented HMAC signing for http push, but I'm not quite sure about a few things.

Was making a custom header a correct approach (like in the python hmac example script)?
Is using OpenSSL ok to calculate HMAC? Is it available on target devices?

I'm sure there's more.

edit: oops.. seems it added the already merged readme by mistake.

robimarko · 2017-07-20T21:01:15Z

Currently there is only python-openssl package preinstalled in images that are generated by nodewatcher

mitar · 2017-07-20T21:20:44Z

Please use a more descriptive branch name and not development. I will rename it for you now.

mitar · 2017-07-20T21:22:40Z

OK, one cannot change source branches. So in the future change this. For now we can leave it like this.

mitar · 2017-07-20T21:24:07Z

Please rebase it on the latest master. You have some commits from before in this branch. For every PR you should create a new clean branch. And every PR should be a rounded unit of contribution. Like in the past you made README improvement. Now it hmac support.

mitar · 2017-07-20T21:26:20Z

Currently there is only python-openssl package preinstalled in images that are generated by nodewatcher

Why would a python package be installed on nodes? Interesting.

mitar · 2017-07-20T21:27:20Z

And no. Do not close the pull request and redo it again. Learn how to rebase an existing branch and do it here and update this branch and pull request.

redmoo · 2017-07-20T22:25:11Z

Oh.. my mistake. Just pushed my local branch. Will have separate descriptive ones from now on.

Hmm, so this means I have to use something other than OpenSSL... I guess installing OpenSSL is not an option?

Will look into rebasing.

mitar · 2017-07-21T00:02:11Z

Hmm, so this means I have to use something other than OpenSSL... I guess installing OpenSSL is not an option?

No, you should check what packages are available and what libraries are available.

robimarko · 2017-07-21T07:01:37Z

@mitar Well it is not preinstalled but rather it is built and available in our image builders.
You can see it on package list in firmware-core

Sent from my Xiaomi Redmi Note 3 using FastHub

mitar · 2017-07-21T08:41:38Z

Yes, but that is different. What we would need here is to know which crypto library is already installed. We have TLS, so which library is used for it. HMAC should use the same one.

BTW, HMAC is very simple. Only a hash function is needed. So maybe we do not need whole crypto library just to have a hash function. So, we should see if we have a hash function anywhere already available.

redmoo · 2017-07-21T12:02:31Z

Libcurl seems to be linked with other libraries to supplement its need for SSL support etc. It's using "gnutls" but I don't think it's available to freely use in the code. Any ideas?

kostko · 2017-07-21T12:10:42Z

BTW, HMAC is very simple. Only a hash function is needed. So maybe we do not need whole crypto library just to have a hash function. So, we should see if we have a hash function anywhere already available.

Exactly, so the whole point of HMAC-based authentication is that it is much simpler than TLS. Because if you have TLS then you can use TLS, but the TLS libraries are quite big. It would be much better if you could either find a small library that implements HMAC-SHA256 or pull in a small HMAC-SHA256 implementation.

redmoo · 2017-07-21T12:15:00Z

Oh.. That makes sense. Will look around.

kostko · 2017-07-21T12:11:34Z

modules/http_push.c

 #if LIBCURL_VERSION_NUM >= 0x072700
        /* Pin server-side public key when configured. */
        char *server_pubkey = nw_uci_get_string(uci, "nodewatcher.@agent[0].push_server_pubkey");
+        char *auth_type = nw_uci_get_string(uci, "nodewatcher.@agent[0].authentication_method");


I would rename this to push_authentication_method.

kostko · 2017-07-21T12:11:43Z

modules/http_push.c

@@ -49,6 +50,7 @@ static int nw_http_push_start_acquire_data(struct nodewatcher_module *module,

  /* Get the configured URL from UCI. */
  char *url = nw_uci_get_string(uci, "nodewatcher.@agent[0].push_url");
+


Why this empty line?

Readability habit I guess... You want me to stop doing that or just wanted an explanation? :)

kostko · 2017-07-21T12:12:53Z

modules/http_push.c

        if (server_pubkey) {
-          curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, 0);
-          curl_easy_setopt(curl, CURLOPT_PINNEDPUBLICKEY, server_pubkey);
+          if (strcmp(auth_type, "hmac") == 0) {


Why is this inslide if (server_pubkey) block? It should be possible to configure no public key, only an HMAC (secret) key.

Ohhh... My bad. Will add another option and separate block.

kostko · 2017-07-21T12:13:43Z

modules/http_push.c

-          curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, 0);
-          curl_easy_setopt(curl, CURLOPT_PINNEDPUBLICKEY, server_pubkey);
+          if (strcmp(auth_type, "hmac") == 0) {
+            unsigned char *hmac_result = HMAC(EVP_sha256(), server_pubkey, strlen(server_pubkey), (unsigned char *)data_string, strlen(data_string), NULL, NULL);


I see, so you are using server_pubkey as the secret key for HMAC. This is not good, you should instead introduce a new configuration option for this new key.

kostko · 2017-07-21T12:14:46Z

modules/http_push.c

@@ -22,6 +22,7 @@

 #include <syslog.h>
 #include <curl.h>
+#include <openssl/hmac.h>


Is there some smaller HMAC implementation available? We don't want to depend on OpenSSL.

Yeah, will look around if there's something appropriate and ask here if that's ok.

kostko · 2017-07-21T12:17:49Z

Currently there is only python-openssl package preinstalled in images that are generated by nodewatcher
Why would a python package be installed on nodes? Interesting.

There are no Python packages installed by default? Only in case you generate firmware for the tunneldigger broker, which is written in Python.

redmoo · 2017-07-21T13:40:39Z

Is something like this still too much and should I look for just an sha256 implementation?

mitar · 2017-07-21T16:02:54Z

You only need one hashing function, like SHA-256. Not the whole library of those.

redmoo · 2017-07-23T11:07:27Z

So.. What is the protocol with adding convenience functions to other people's code? Depending on licence? Because it would be really unfortunate if I legally couldn't change bad macro definitions or have to add related methods to other files...

mitar · 2017-07-23T11:17:30Z

So.. What is the protocol with adding convenience functions to other people's code? Depending on licence? Because it would be really unfortunate if I legally couldn't change bad macro definitions or have to add related methods to other files...

What do you mean? Which code?

redmoo · 2017-07-23T11:21:28Z

sha256 implementation from someone else. I will find another one anyway but it would be good to know if I can dabble in it.

mitar · 2017-07-23T11:26:16Z

I will find another one anyway but it would be good to know if I can dabble in it.

I mean, as you said. Depending on the license of that code. But probably if the license does not allow you to change the code, the we do not want that code in our code anyway.

redmoo · 2017-07-23T11:28:03Z

Hah.. Good point. Ok. I'll push it all anyway so people can comment... The sha256 implementation will be changed though.

robimarko · 2017-07-23T21:18:03Z

As far as I can see you are using this library?
https://github.com/B-Con/crypto-algorithms

If that is the case then the author has listed this as licence info:
This code is released into the public domain free of any restrictions. The author requests acknowledgement if the code is used, but does not require it. This code is provided free of any liability and without any quality claims by the author.

But the author himself has stated this:
Note that these are not cryptographically secure implementations. They have no resistence to side-channel attacks and should not be used in contexts that need cryptographically secure implementations.

These algorithms are not optimized for speed or space. They are primarily designed to be easy to read, although some basic optimization techniques have been employed

Therefor I dont think these are really good for use

redmoo · 2017-07-23T21:25:27Z

No it is not a good option, but I commited it anyway so you can all comment about the hmac portion or http_push part. I found another proper solution but it's way too fragmented and didn't even work correctly (failed the tests), so I'm still looking for a good option to replace this sha implementation.

robimarko · 2017-07-23T21:27:04Z

I understand.

redmoo · 2017-07-24T12:27:51Z

So... After some reading and one unsuccessful porting I have a dilemma. The current implementation is supposed to be vulnerable to side channel attacks. Ok. But so is openssl and many other libraries, especially when using the default C implementation (plus you need physical access to the device). It doesn't seem like it's as easy as porting a bit of code from some library to fix it. It depends on other libraries for memory access etc. Really time consuming if we are to get any benefit from it.

I propose that we use this for now as it does work, pardon the phrase, "good enough" and I focus on my remaining duties. This can still be used and upgraded at a later date. Which I can do, after other assignments are done.

kostko · 2017-07-24T15:55:52Z

common/hmac.c

+    sha256_final(&ctx, buf);
+}
+
+void hmac(BYTE *key, int key_len, const BYTE *data, int data_len, BYTE *hmac_out)


This is HMAC-SHA256, name accoordingly (e.g. hmac_sha256).

kostko · 2017-07-24T15:56:05Z

common/hmac.c

+    sha256(in_buf, hmac_out);
+
+    free(in_buf);
+}


Add newline at end of file.

Also elsewhere.

sha256() wrapper only works correctly for strings.

Why the XORs? Oh, I see, because sha256() doesn't work correctly when it encounters \0.

Assembling buffer copies into newly allocated buffers should not be needed - just use the primitives from sha256()

key_len == SHA256_BLOCK_SIZE case not handled.

Finally, why reinventing the wheel?

kostko · 2017-07-24T15:57:37Z

modules/http_push.c

-        curl_easy_setopt(curl, CURLOPT_POSTFIELDS, json_object_to_json_string(data));
+        curl_easy_setopt(curl, CURLOPT_POSTFIELDS, data_string);
+
+        const char *auth_type = nw_uci_get_string(uci, "nodewatcher.@agent[0].push_authentication_method"); // TODO: is hmac default?


No, it should be backwards compatible so default should be as before (public key).

kostko · 2017-07-24T15:58:35Z

modules/http_push.c

+
+          if (hmac_key) {
+            unsigned char hmac_out[SHA256_HASH_SIZE];
+            hmac((unsigned char *)hmac_key, strlen(hmac_key), (unsigned char *)data_string, strlen(data_string), hmac_out);


No space before pointer, space after cast, e.g. (unsigned char*) hmac_key. Also elsewhere.

Ok. Just to nitpick... Elsewhere it's always "char *var", so it made sense to me to keep it the same way. :)

redmoo · 2017-07-24T19:18:06Z

Before merging, what about licence? I didn't paste anything to my new files... Should I have? I guess the same as in the other files with updated name etc..

domenpk

hmac appears to be broken, since it's limited to C string input.

other small comments

domenpk · 2017-10-22T08:36:04Z

common/hmac.c

+    sha256(in_buf, hmac_out);
+
+    free(in_buf);
+}


sha256() wrapper only works correctly for strings.

Why the XORs? Oh, I see, because sha256() doesn't work correctly when it encounters \0.

Assembling buffer copies into newly allocated buffers should not be needed - just use the primitives from sha256()

key_len == SHA256_BLOCK_SIZE case not handled.

Finally, why reinventing the wheel?

domenpk · 2017-10-22T08:40:29Z

modules/http_push.c

+
+            char signature[SHA256_HASH_SIZE + 1] = {0};
+
+            if (nw_base64_encode(hmac_out, SHA256_HASH_SIZE, signature, SHA256_HASH_SIZE) == 0) {


base64 inflates the output size, so this probably isn't correct.

domenpk · 2017-10-22T08:42:31Z

modules/http_push.c

+
+            if (nw_base64_encode(hmac_out, SHA256_HASH_SIZE, signature, SHA256_HASH_SIZE) == 0) {
+              char signature_header[26+sizeof(signature)] = {0};
+              strcat(strcpy(signature_header, "X-Nodewatcher-Signature: "), signature);


Odd at least. Can you use snprintf?
Or, how about just initializing to "X-Nodewatcher-Signature: " instead of {0}?

redmoo added 4 commits June 26, 2017 06:35

Added nodewatcher-agent development section to README.

9d0572d

Updated README styling.

0009463

Added commit hash warning and subsections to README.

a44c43e

Added LEDE repository link.

8304689

redmoo added enhancement help wanted labels Jul 20, 2017

redmoo self-assigned this Jul 20, 2017

redmoo requested review from kostko and mitar July 20, 2017 20:51

redmoo force-pushed the development branch from fdce459 to 8304689 Compare July 21, 2017 00:33

redmoo added 2 commits July 20, 2017 17:36

Merge branch 'master' into development

8f92420

HMAC signing for http push using OpenSSL.

da088ef

kostko reviewed Jul 21, 2017

View reviewed changes

Separate HMAC code block, HMAC key setting, variable renaming.

9489e58

Hmac implementation, temporary sha256, updated http_push module.

8453843

kostko reviewed Jul 24, 2017

View reviewed changes

Style changes, variable renaming.

7abca81

Added missing licence to new files.

7e13185

domenpk requested changes Oct 22, 2017

View reviewed changes

		@@ -49,6 +50,7 @@ static int nw_http_push_start_acquire_data(struct nodewatcher_module *module,

		/* Get the configured URL from UCI. */
		char *url = nw_uci_get_string(uci, "nodewatcher.@agent[0].push_url");


		char signature[SHA256_HASH_SIZE + 1] = {0};

		if (nw_base64_encode(hmac_out, SHA256_HASH_SIZE, signature, SHA256_HASH_SIZE) == 0) {

HMAC signing for http push #5

Are you sure you want to change the base?

HMAC signing for http push #5

Conversation

redmoo commented Jul 20, 2017 • edited Loading

This is part of my contribution to Google Summer of Code 2017.

robimarko commented Jul 20, 2017

mitar commented Jul 20, 2017

mitar commented Jul 20, 2017

mitar commented Jul 20, 2017

mitar commented Jul 20, 2017

mitar commented Jul 20, 2017

redmoo commented Jul 20, 2017

mitar commented Jul 21, 2017

robimarko commented Jul 21, 2017

mitar commented Jul 21, 2017 • edited Loading

redmoo commented Jul 21, 2017

kostko commented Jul 21, 2017

redmoo commented Jul 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostko commented Jul 21, 2017

redmoo commented Jul 21, 2017 • edited Loading

mitar commented Jul 21, 2017

redmoo commented Jul 23, 2017

mitar commented Jul 23, 2017

redmoo commented Jul 23, 2017

mitar commented Jul 23, 2017

redmoo commented Jul 23, 2017

robimarko commented Jul 23, 2017

redmoo commented Jul 23, 2017

robimarko commented Jul 23, 2017

redmoo commented Jul 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redmoo commented Jul 24, 2017

domenpk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redmoo commented Jul 20, 2017 •

edited

Loading

mitar commented Jul 21, 2017 •

edited

Loading

redmoo commented Jul 21, 2017 •

edited

Loading

redmoo commented Jul 24, 2017 •

edited

Loading