Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Request for BearSSL library support. #95

Closed
hdrut opened this issue Jul 26, 2018 · 22 comments
Closed

Request for BearSSL library support. #95

hdrut opened this issue Jul 26, 2018 · 22 comments
Labels

Comments

@hdrut
Copy link

hdrut commented Jul 26, 2018

Hi, I know you are really busy, but please consider adding support for BearSSL as it's apparently going to be the default choice for Arduino TLS. I think @Adam5Wu has done some interesting progress.
Thank you.

@arihantdaga
Copy link

@Adam5Wu Can you please share something with us... If your fork is working fine and can be tested.. ?

@Adam5Wu
Copy link
Contributor

Adam5Wu commented Sep 2, 2018

Hi @arihantdaga the situation of my fork is a little complicated. Yes, the client side function is working. But you must use my fork of Arduino, because the official fork have merged a version of Bearssl that is only compatible with synchronized workflow.

However my fork of Arduino is sliding behind the upstream, due to another problem. The upstream introduced another feature which moves the sketch stack into the SDK reserved stack space to save 4KB of memory. However I found this causes problems with certain SDK functions such as WPS and maybe WPA2 Enterprise.

In my Arduino fork I have undone the stack changes, however I think it may drift too far away from the official Arduino upstream and cause maintenance problem. I hope the upstream will eventually make the stack location configurable so I don't have to maintain too much of my own private changes in my fork. So I am just waiting at the moment...

@tve
Copy link

tve commented Oct 20, 2018

Is there an open issue to fix the "a version of Bearssl that is only compatible with synchronized workflow" problem?

@Adam5Wu
Copy link
Contributor

Adam5Wu commented Oct 21, 2018

I think the latest revision of BearSSL by @earlephilhower should be compatible with both cont and async, just I do not have time to integrate with my fork. Maybe in a couple of weeks...

@thekurtovic
Copy link

I have installed @Adam5Wu 's fork of Arduino, and placed his modified versions of ESPAsyncTCP, and Async-MQTT-Client in the "libraries" folder, everything compiles just fine. However, I am running into issues where I cannot connect to a broker using SSL, though non-SSL works fine. I believe async_config.h (from ESPAsyncTCP) is configured properly, and I am setting the MQTT client to be secure.
#define ASYNC_TCP_SSL_ENABLED 1
#define ASYNC_TCP_SSL_BEARSSL 1
mqttClient.setSecure(true);

The onMqttDisconnect callback appears to fire immediately after attempting to connect. I am using a free instance of CloudMQTT to test the modified files by @Adam5Wu , and this is what is shown in the log.
2018-12-09 19:04:00: New connection from 174.89.112.159 on port 23059.
2018-12-09 19:04:00: Socket error on client , disconnecting.

@hdrut @arihantdaga @tve Have any of you tried implementing the modified Async-MQTT-Client library, and successfully connected with SSL? I'm not exactly sure what I am missing, but any help would be much appreciated.

@Adam5Wu
Copy link
Contributor

Adam5Wu commented Dec 10, 2018

@thekurtovic I think you maybe missing the cert validation callbacks.
For any SSL connection to successfully establish, server side certification must be validated (otherwise it is like someone calling you claiming they are IRS and ask you wire your "overdue tax"...).

By default there was no validation, and all connections are automatically rejected. Yes it is quite "unfriendly", and I should spend more time to improve it. But at least it is safe, compare to making the default accept anything -- people may unknowingly put the broken logic in their door sensor code and broadcast to the Internet that no one is at home tonight...

Anyway, to successfully establish TLS connection, please do the following:

mqClient->onSSLCertLookup(match_cert);
...
int match_cert(void *dn_hash, size_t dn_hash_len, uint8_t **buf) {
  if (dn_hash matches a well-known certificate hash) {
    *buf = (pointer to the well-known certificate blob)
    return (length of the certificate blob);
  }
  PrintString HashStr;
  for (int i = 0; i < dn_hash_len; i++) {
    HashStr.printf("%02X", ((char*)dn_hash)[i]);
  }
  LOG("Unmatched certificate: %s\n", HashStr.c_str());
  return 0;
}
  • You should have a PEM encoded server certificate, or any certificate along the trust chain (e.g. the issuer certificate of the server certificate, or the root CA's certificate)
  • The certificate hash is generated by BearSSL's own internal logic, so at first you may not know what it is.
    • You can run the code without providing a valid hash, the connection won't go through, but the LOG message will give you a clue of what the hash should be
    • If you have the server's cert, then the first unmatched hash should be its hash code
    • If you have the issuer cert of the server cert, then the second (unique) unmatched hash
    • If you have the root cert, then the last (unique) unmatched hash

Sorry for the rough API interface, @earlephilhower did a much better job providing API for operating a repository of certificates which makes cert matching easier.

@Adam5Wu
Copy link
Contributor

Adam5Wu commented Dec 10, 2018

Here is a piece of code that I have in one of my own private project. I haven't published it because I haven't completed the first round implementation, but given my current work & life schedule, it may take longer time to reach the finish line...

Edit: The following code partially referred to @earlephilhower's certificate API. The main difference is that the actual certificate loading operation is deferred into the timer callback context. The reason is that, to conserve memory resource, async operated BearSSL does not allocate 2nd stack, instead only use sys stack, which is available in callbacks.

Anyway, this code provide a slightly easier to use wrapper for cert validation:

class TLSCert {
  protected:
    String tlsDNHash;
    String tlsCertData;

  public:
    TLSCert(String &&dn_hash, String &&cert_data)
		: tlsDNHash(std::move(dn_hash)), tlsCertData(std::move(cert_data)) {}
	
    bool match(void *dn_hash, size_t dn_hash_len) {
      return ((dn_hash_len == tlsDNHash.length()) &&
              (memcmp(dn_hash, tlsDNHash.begin(), dn_hash_len) == 0));
    }
    uint8_t* data() {
      return (uint8_t*)tlsCertData.begin();
    }
    size_t length() {
      return tlsCertData.length();
    }

    static TLSCert* load(Stream &src);
};

struct PEMObject {
  String tag;
  String data;
};

typedef LinkedList<PEMObject> PEMObjects;

#define STREAM_BUFLEN 512

static void pem_append(void *ctx, const void *buf, size_t len) {
  auto data = static_cast<String *>(ctx);
  data->concat((const char*)buf, len);
}

static PEMObjects decode_pem(Stream &src, int limit = 0) {
  br_pem_decoder_context pc;

  PEMObject po;
  PEMObjects Ret(nullptr);

  char buff[STREAM_BUFLEN];
  size_t bufflen = 0;
  size_t buffpos = 0;

  br_pem_decoder_init(&pc);
  bool decoder_continue = true;

  while (decoder_continue) {
    if (buffpos >= bufflen) {
      buffpos = 0;
      if (src.available()) {
        bufflen = src.readBytes(buff, STREAM_BUFLEN);
        if (bufflen == -1) {
          LOGVV("WARNING: Error loading PEM data\n");
          break;
        }
      } else {
        decoder_continue = false;
        buff[0] = '\n';
        bufflen = 1;
      }
    }
    size_t tlen = br_pem_decoder_push(&pc, buff + buffpos, bufflen - buffpos);
    buffpos += tlen;
    switch (br_pem_decoder_event(&pc)) {
      case BR_PEM_BEGIN_OBJ:
        // Feed the dog before it bites
        system_soft_wdt_feed();
        po.tag = br_pem_decoder_name(&pc);
        LOGVV("PEM object [%s]\n", po.tag.c_str());
        br_pem_decoder_setdest(&pc, pem_append, &po.data);
        break;

      case BR_PEM_END_OBJ:
        if (po.data) {
          LOGVV("PEM object length %d\n", po.data.length());
          Ret.append(std::move(po));
          if (limit && Ret.length() >= limit)
            decoder_continue = false;
        } else {
          LOG("WARNING: PEM object end marker without content\n");
        }
        break;

      case BR_PEM_ERROR:
        LOG("WARNING: Error decoding PEM object\n");
        decoder_continue = false;
        break;

      default:
        // Do nothing here, the parser is still working on things
        break;
    }
  }

  if (po.tag) {
    LOG("WARNING: Discarding partial PEM object of length %d\n", po.data.length());
  }
  return std::move(Ret);
}

static void dn_append(void *ctx, const void *buf, size_t len) {
  br_sha256_context *hasher = (br_sha256_context*)ctx;
  br_sha256_update(hasher, buf, len);
}

typedef uint8_t SHA256[32];
bool hash_cert(Stream &src, SHA256 &hash) {
  br_x509_decoder_context decoder;
  br_sha256_context hasher;
  br_sha256_init(&hasher);
  br_x509_decoder_init(&decoder, dn_append, &hasher, nullptr, nullptr);

  char buff[STREAM_BUFLEN];
  size_t bufflen = 0;

  while (src.available()) {
    bufflen = src.readBytes(buff, STREAM_BUFLEN);
    if (bufflen == -1) {
      LOGVV("WARNING: Error loading certificate data\n");
      break;
    }
    br_x509_decoder_push(&decoder, buff, bufflen);
  }
  if (int err = br_x509_decoder_last_error(&decoder)) {
    LOGDO({
      const char *desc;
      if (find_error_name(err, &desc)) {
        ESPSSLHLP_LOG("Certificate decoding error - %s\n", SFPSTR(desc));
      } else {
        ESPSSLHLP_LOG("Certificate decoding error (%d)\n", err);
      }
    });
    return false;
  }
  br_sha256_out(&hasher, hash);
  LOGVDO({
    PrintString HashStr;
    for (int i = 0; i < sizeof(SHA256); i++) {
      HashStr.printf("%02X", ((char*)hash)[i]);
    }
    ESPSSLHLP_LOG("Trusted certificate: %s\n", HashStr.c_str());
  });
  return true;
}

TLSCert* load_cert(Stream &src) {
  PEMObjects BinCert = decode_pem(src);
  if (BinCert.length()) {
    LOG("Decoded %d PEM objects\n", BinCert.length());
    auto Cert = BinCert.get_if([](PEMObject const &obj) {
      return obj.tag == FC("CERTIFICATE") || obj.tag == FC("X509 CERTIFICATE");
    });
    if (Cert) {
      // Feed the dog before it bites
      system_soft_wdt_feed();

      SHA256 CertHash;
      StreamString CertData(std::move(Cert->data));
      if (hash_cert(CertData, CertHash)) {
        return new TLSCert(String((const char*)CertHash, sizeof(SHA256)), std::move(CertData));
      } else {
        LOG("WARNING: Failed to process certificate\n");
      }
    } else {
      LOG("WARNING: No certificate found in PEM objects\n");
    }
  } else {
    LOG("WARNING: No valid PEM objects decoded\n");
  }
  return nullptr;
}

#ifdef BEARSSL_ALTSTACK
TLSCert* TLSCert::load(Stream &src) {
  return load_cert(src);
}
#else

extern "C" {
  void esp_schedule();
  void esp_yield();
}

static TLSCert * volatile __cert__;
static Ticker __ticker__;

static void __do_load_cert__(Stream *src) {
	__cert__ = load_cert(*src);
	esp_schedule();
}

TLSCert* TLSCert::load(Stream &src) {
	__ticker__.once_ms(0, __do_load_cert__, &src);
	esp_yield();
	return __cert__;
}

To use this class with the server cert validation procedure:

struct {
  AsyncMqttClient *mqClient;
  TLSCert *mqCert;  // Transient object, created during connection, removed upon connected or disconnected
} State;

int match_cert(void *dn_hash, size_t dn_hash_len, uint8_t **buf) {
  if (State.mqCert && State.mqCert->match(dn_hash, dn_hash_len)) {
    LOG("Trusted certificate matched with remote server!\n");
    *buf = State.mqCert->data();
    return State.mqCert->length();
  }
	PrintString HashStr;
	for (int i = 0; i < dn_hash_len; i++) {
		HashStr.printf("%02X", ((char*)dn_hash)[i]);
	}
	LOG("Unmatched certificate: %s\n", HashStr.c_str());
  return 0;
}

void clear_cert() {
  delete State.mqCert;
  State.mqCert = nullptr;
}

void onMqttConnect(bool sessionPresent) {
  clear_cert();
  //...
}

void onMqttDisconnect(AsyncMqttClientDisconnectReason reason) {
  clear_cert();
  //...
}

auto CertFile = fs.openFile(PATH_TO_CERT_FILE, "r");
State.mqCert = TLSCert::load(CertFile);

State.mqClient = new AsyncMqttClient();
State.mqClient->setSecure(true);
State.mqClient->onConnect(onMqttConnect);
State.mqClient->onDisconnect(onMqttDisconnect);
State.mqClient->onSSLCertLookup(match_cert);
  • Simply put the PEM encoded server cert, or any cert along the trust chain, into a file
  • Define PATH_TO_CERT_FILE to point to the cert file

@thekurtovic
Copy link

@Adam5Wu Thank you for taking the time to provide such a detailed response. I appreciate the information which you have shared. Your assumption was correct, I was not making use of certificate validation callbacks.

Over the last few days I've been trying to wrap my head around the certificate validation wrapper which you provided, but I think it may be too much for me to understand. I have been trying to resolve each compiler error I am seeing, by including the libraries which I seem to be missing, and changing your LOG commands to Serial.printf/println, but I've not been able to successfully compile a test sketch. I'm not sure where things like br_pem_decoder_context/init/event/name, or br_Sha256_context are originating from to name a few examples.

@earlephilhower
Copy link

@Adam5Wu, drop me a note if there's something I can change in the BearSSL port to make it easier for you to use with AsyncTCP. I thought the thunking stuff would actually help you out, but I guess not.

@Adam5Wu
Copy link
Contributor

Adam5Wu commented Dec 19, 2018

Hi @earlephilhower, actually the thunking does help (although I recently had no time to try, but I am pretty confident it will work perfectly with async).

The problem with certificate validation is that, opening a certificate would require some heavy crypto operations, which needs lots of stack, and the g_cont stack may not be enough.

There are two choices:

  1. Allocate and use a larger 2nd stack, and
  2. Use sys context stack.

In non-async esp8266 Arduino, with g_cont stack relocated to overlap sys context stack, one can choose approach 1; But in async workflow, because lots of activities happen in the callback context, including heavy crypto, I think it is best not to relocate g_cont stack, instead just use sys context stack directly.

Now the problem comes when one needs to open a certificate in g_cont context:

  1. The g_cont stack is not large enough, and
  2. If we allocate 2nd stack, then effectively there will be three stacks: 8KB sys context, 4KB g_cont, and ~6KB crypto stack. And that may be just too much for esp8266's tiny free RAM...

So instead, I try to queue certificate operations in a callback, effectively convert a sync job into pseudo-async, so that it can directly use the sys context. It can save ~6KB of memory.

In other words, once one decided to use async style bearSSL, then EVERY and ALL bearSSL operations must be in async style, which is why your existing implementations of certificate helpers cannot be directly used.

@Adam5Wu
Copy link
Contributor

Adam5Wu commented Dec 19, 2018

@Adam5Wu Thank you for taking the time to provide such a detailed response. I appreciate the information which you have shared. Your assumption was correct, I was not making use of certificate validation callbacks.

Over the last few days I've been trying to wrap my head around the certificate validation wrapper which you provided, but I think it may be too much for me to understand. I have been trying to resolve each compiler error I am seeing, by including the libraries which I seem to be missing, and changing your LOG commands to Serial.printf/println, but I've not been able to successfully compile a test sketch. I'm not sure where things like br_pem_decoder_context/init/event/name, or br_Sha256_context are originating from to name a few examples.

@thekurtovic Sorry, forgot to putting the include while extracting code.
Hopefully the following works for you:

#include <user_interface.h>

#include <Stream.h>
#include <WString.h>
#include <StreamString.h>

#include <include/bearssl.h>
#include <brssl.h>
#include <LinkedList.h>
#include <Ticker.h>

@Niek
Copy link

Niek commented Apr 12, 2019

Is the situation any different now that core 2.5.0 has shipped with BearSSL as default?

@stale
Copy link

stale bot commented Sep 21, 2019

[STALE_SET] This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Sep 21, 2019
@Niek
Copy link

Niek commented Sep 21, 2019

FYI there's some work being done on this by @mcspr in https://github.com/mcspr/ESPAsyncTCP/tree/bearssl

@stale
Copy link

stale bot commented Sep 21, 2019

[STALE_CLR] This issue has been removed from the stale queue. Please ensure activity to keep it openin the future.

@stale stale bot removed the stale label Sep 21, 2019
@tve
Copy link

tve commented Sep 21, 2019

I assume you saw #48? Or is there a reason it has to be bearssl?

@mcspr
Copy link

mcspr commented Sep 23, 2019

It is already packaged by the framework, so why not?
*me-no-dev/AsyncTCP#48 - i do see mbedtls in https://github.com/espressif/ESP8266_RTOS_SDK/ repo too, but we are still using nonos stuff
I would think it would be possible to directly use current esp-idf version of AsyncClient instead of this repo, no idea what is the planned timeline for migration though

@tve
Copy link

tve commented Sep 23, 2019

Sorry, my bad, got esp8266 ESPAsyncTCP and esp32 AsyncTCP mixed-up (I get notifications for both in my inbox).

@stale
Copy link

stale bot commented Nov 22, 2019

[STALE_SET] This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Nov 22, 2019
@stale
Copy link

stale bot commented Dec 6, 2019

[STALE_DEL] This stale issue has been automatically closed. Thank you for your contributions.

@stale stale bot closed this as completed Dec 6, 2019
@s00500
Copy link

s00500 commented Dec 10, 2019

Oh stale bot.... So I am currently running into an issue with this library where it would screw up tls1.2 and hang... (but works fine with tls1.1) so it would be interesting to try if bearssl would solve this.... need to do further debugging though... but this issue should definitly not be closed by the stale bot ;-)

@jeroenst
Copy link

How much memory would be saved using bearssl instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

10 participants