[Asterisk-code-review] AST-2022-001 - res_stir_shaken/curl: Limit file size and check start. (asterisk[16])
Friendly Automation
asteriskteam at digium.com
Thu Apr 14 12:20:47 CDT 2022
Friendly Automation has uploaded this change for review. ( https://gerrit.asterisk.org/c/asterisk/+/18379 )
Change subject: AST-2022-001 - res_stir_shaken/curl: Limit file size and check start.
......................................................................
AST-2022-001 - res_stir_shaken/curl: Limit file size and check start.
Put checks in place to limit how much we will actually download, as well
as a check for the data we receive at the start to ensure it begins with
what we would expect a certificate to begin with.
ASTERISK-29872
Change-Id: Ifd3c6b8bd52b8b6192a04166ccce4fc8a8000b46
---
M res/res_stir_shaken/curl.c
M res/res_stir_shaken/stir_shaken.c
M res/res_stir_shaken/stir_shaken.h
3 files changed, 107 insertions(+), 60 deletions(-)
git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/79/18379/1
diff --git a/res/res_stir_shaken/curl.c b/res/res_stir_shaken/curl.c
index b31160f..cd78461 100644
--- a/res/res_stir_shaken/curl.c
+++ b/res/res_stir_shaken/curl.c
@@ -31,12 +31,27 @@
/* Used to check CURL headers */
#define MAX_HEADER_LENGTH 1023
+/* Used to limit download size */
+#define MAX_DOWNLOAD_SIZE 8192
+
+/* Used to limit how many bytes we get from CURL per write */
+#define MAX_BUF_SIZE_PER_WRITE 1024
+
+/* Certificates should begin with this */
+#define BEGIN_CERTIFICATE_STR "-----BEGIN CERTIFICATE-----"
+
/* CURL callback data to avoid storing useless info in AstDB */
struct curl_cb_data {
char *cache_control;
char *expires;
};
+struct curl_cb_write_buf {
+ char buf[MAX_DOWNLOAD_SIZE + 1];
+ size_t size;
+ const char *url;
+};
+
struct curl_cb_data *curl_cb_data_create(void)
{
struct curl_cb_data *data;
@@ -149,94 +164,132 @@
return curl;
}
+/*!
+ * \brief Write callback passed to libcurl
+ *
+ * \note If this function returns anything other than the size of the data
+ * libcurl expected us to process, the request will cancel. That's why we return
+ * 0 on error, otherwise the amount of data we were given
+ *
+ * \param curl_data The data from libcurl
+ * \param size Always 1 according to libcurl
+ * \param actual_size The actual size of the data
+ * \param our_data The data we passed to libcurl
+ *
+ * \retval The size of the data we processed
+ * \retval 0 if there was an error
+ */
+static size_t curl_write_cb(void *curl_data, size_t size, size_t actual_size, void *our_data)
+{
+ /* Just in case size is NOT always 1 or if it's changed in the future, let's go ahead
+ * and do the math for the actual size */
+ size_t real_size = size * actual_size;
+ struct curl_cb_write_buf *buf = our_data;
+ size_t new_size = buf->size + real_size;
+
+ if (new_size > MAX_DOWNLOAD_SIZE) {
+ ast_log(LOG_WARNING, "Attempted to retrieve certificate from %s failed "
+ "because it's size exceeds the maximum %d bytes\n", buf->url, MAX_DOWNLOAD_SIZE);
+ return 0;
+ }
+
+ memcpy(&(buf->buf[buf->size]), curl_data, real_size);
+ buf->size += real_size;
+ buf->buf[buf->size] = 0;
+
+ return real_size;
+}
+
char *curl_public_key(const char *public_cert_url, const char *path, struct curl_cb_data *data)
{
FILE *public_key_file;
- RAII_VAR(char *, tmp_filename, NULL, ast_free);
- const char *template_name = "certXXXXXX";
char *filename;
char *serial;
- int fd;
long http_code;
CURL *curl;
char curl_errbuf[CURL_ERROR_SIZE + 1];
+ struct curl_cb_write_buf *buf;
+ buf = ast_calloc(1, sizeof(*buf));
+ if (!buf) {
+ ast_log(LOG_ERROR, "Failed to allocate memory for CURL write buffer for %s\n", public_cert_url);
+ return NULL;
+ }
+
+ buf->url = public_cert_url;
curl_errbuf[CURL_ERROR_SIZE] = '\0';
- /* For now, it's fine to pass in path as is - it shouldn't end with a '/'. However,
- * if we decide to change how certificates are stored in the future (configurable paths),
- * then we will need to check to see if path ends with '/', copy everything up to the '/',
- * and use this new variable for ast_create_temp_file as well as for ast_asprintf below.
- */
- fd = ast_file_fdtemp(path, &tmp_filename, template_name);
- if (fd == -1) {
- ast_log(LOG_ERROR, "Failed to get temporary file descriptor for CURL\n");
- return NULL;
- }
-
- public_key_file = fdopen(fd, "wb");
- if (!public_key_file) {
- ast_log(LOG_ERROR, "Failed to open file '%s' to write public key from '%s': %s (%d)\n",
- tmp_filename, public_cert_url, strerror(errno), errno);
- close(fd);
- remove(tmp_filename);
- return NULL;
- }
-
curl = get_curl_instance(data);
if (!curl) {
ast_log(LOG_ERROR, "Failed to set up CURL instance for '%s'\n", public_cert_url);
- fclose(public_key_file);
- remove(tmp_filename);
+ ast_free(buf);
return NULL;
}
curl_easy_setopt(curl, CURLOPT_URL, public_cert_url);
- curl_easy_setopt(curl, CURLOPT_WRITEDATA, public_key_file);
+ curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, curl_write_cb);
+ curl_easy_setopt(curl, CURLOPT_WRITEDATA, buf);
curl_easy_setopt(curl, CURLOPT_ERRORBUFFER, curl_errbuf);
+ curl_easy_setopt(curl, CURLOPT_BUFFERSIZE, MAX_BUF_SIZE_PER_WRITE);
if (curl_easy_perform(curl)) {
ast_log(LOG_ERROR, "%s\n", curl_errbuf);
curl_easy_cleanup(curl);
- fclose(public_key_file);
- remove(tmp_filename);
+ ast_free(buf);
return NULL;
}
curl_easy_getinfo(curl, CURLINFO_RESPONSE_CODE, &http_code);
curl_easy_cleanup(curl);
- fclose(public_key_file);
if (http_code / 100 != 2) {
ast_log(LOG_ERROR, "Failed to retrieve URL '%s': code %ld\n", public_cert_url, http_code);
- remove(tmp_filename);
+ ast_free(buf);
return NULL;
}
- serial = stir_shaken_get_serial_number_x509(tmp_filename);
+ if (!ast_begins_with(buf->buf, BEGIN_CERTIFICATE_STR)) {
+ ast_log(LOG_WARNING, "Certificate from %s does not begin with what we expect\n", public_cert_url);
+ ast_free(buf);
+ return NULL;
+ }
+
+ serial = stir_shaken_get_serial_number_x509(buf->buf, buf->size);
if (!serial) {
- ast_log(LOG_ERROR, "Failed to get serial from cert %s\n", tmp_filename);
- remove(tmp_filename);
+ ast_log(LOG_ERROR, "Failed to get serial from CURL buffer from %s\n", public_cert_url);
+ ast_free(buf);
return NULL;
}
if (ast_asprintf(&filename, "%s/%s.pem", path, serial) < 0) {
- ast_log(LOG_ERROR, "Failed to allocate memory for new filename for temporary "
- "file %s after CURL\n", tmp_filename);
+ ast_log(LOG_ERROR, "Failed to allocate memory for filename after CURL from %s\n", public_cert_url);
ast_free(serial);
- remove(tmp_filename);
+ ast_free(buf);
return NULL;
}
ast_free(serial);
- if (rename(tmp_filename, filename)) {
- ast_log(LOG_ERROR, "Failed to rename temporary file %s to %s after CURL\n", tmp_filename, filename);
+ public_key_file = fopen(filename, "w");
+ if (!public_key_file) {
+ ast_log(LOG_ERROR, "Failed to open file '%s' to write public key from '%s': %s (%d)\n",
+ filename, public_cert_url, strerror(errno), errno);
+ ast_free(buf);
ast_free(filename);
- remove(tmp_filename);
return NULL;
}
+ if (fputs(buf->buf, public_key_file) == EOF) {
+ ast_log(LOG_ERROR, "Failed to write string to file from URL %s\n", public_cert_url);
+ fclose(public_key_file);
+ ast_free(buf);
+ ast_free(filename);
+ return NULL;
+ }
+
+ fclose(public_key_file);
+ ast_free(buf);
+
return filename;
}
diff --git a/res/res_stir_shaken/stir_shaken.c b/res/res_stir_shaken/stir_shaken.c
index 6bc07ea..9e17f7c 100644
--- a/res/res_stir_shaken/stir_shaken.c
+++ b/res/res_stir_shaken/stir_shaken.c
@@ -137,41 +137,35 @@
return key;
}
-char *stir_shaken_get_serial_number_x509(const char *path)
+char *stir_shaken_get_serial_number_x509(const char *buf, size_t buf_size)
{
- FILE *fp;
+ BIO *certBIO;
X509 *cert;
ASN1_INTEGER *serial;
BIGNUM *bignum;
char *serial_hex;
char *ret;
- fp = fopen(path, "r");
- if (!fp) {
- ast_log(LOG_ERROR, "Failed to open file %s\n", path);
- return NULL;
- }
-
- cert = PEM_read_X509(fp, NULL, NULL, NULL);
+ certBIO = BIO_new(BIO_s_mem());
+ BIO_write(certBIO, buf, buf_size);
+ cert = PEM_read_bio_X509(certBIO, NULL, NULL, NULL);
+ BIO_free(certBIO);
if (!cert) {
- ast_log(LOG_ERROR, "Failed to read X.509 cert from file %s\n", path);
- fclose(fp);
+ ast_log(LOG_ERROR, "Failed to read X.509 cert from buffer\n");
return NULL;
}
serial = X509_get_serialNumber(cert);
if (!serial) {
- ast_log(LOG_ERROR, "Failed to get serial number from certificate %s\n", path);
+ ast_log(LOG_ERROR, "Failed to get serial number from certificate\n");
X509_free(cert);
- fclose(fp);
return NULL;
}
bignum = ASN1_INTEGER_to_BN(serial, NULL);
if (bignum == NULL) {
- ast_log(LOG_ERROR, "Failed to convert serial to bignum for certificate %s\n", path);
+ ast_log(LOG_ERROR, "Failed to convert serial to bignum for certificate\n");
X509_free(cert);
- fclose(fp);
return NULL;
}
@@ -181,18 +175,17 @@
*/
serial_hex = BN_bn2hex(bignum);
X509_free(cert);
- fclose(fp);
BN_free(bignum);
if (!serial_hex) {
- ast_log(LOG_ERROR, "Failed to convert bignum to hex for certificate %s\n", path);
+ ast_log(LOG_ERROR, "Failed to convert bignum to hex for certificate\n");
return NULL;
}
ret = ast_strdup(serial_hex);
OPENSSL_free(serial_hex);
if (!ret) {
- ast_log(LOG_ERROR, "Failed to dup serial from openssl for certificate %s\n", path);
+ ast_log(LOG_ERROR, "Failed to dup serial from openssl for certificate\n");
return NULL;
}
diff --git a/res/res_stir_shaken/stir_shaken.h b/res/res_stir_shaken/stir_shaken.h
index 90df4e9..a707c3b 100644
--- a/res/res_stir_shaken/stir_shaken.h
+++ b/res/res_stir_shaken/stir_shaken.h
@@ -53,15 +53,16 @@
EVP_PKEY *stir_shaken_read_key(const char *path, int priv);
/*!
- * \brief Gets the serial number in hex form from the X509 certificate at path
+ * \brief Gets the serial number in hex form from the buffer (for X509)
*
* \note The returned string will need to be freed by the caller
*
- * \param path The full path of the X509 certificate
+ * \param buf The BASE64 encoded buffer
+ * \param buf_size The size of the data in buf
*
* \retval NULL on failure
* \retval serial number on success
*/
-char *stir_shaken_get_serial_number_x509(const char *path);
+char *stir_shaken_get_serial_number_x509(const char *buf, size_t buf_size);
#endif /* _STIR_SHAKEN_H */
--
To view, visit https://gerrit.asterisk.org/c/asterisk/+/18379
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: Ifd3c6b8bd52b8b6192a04166ccce4fc8a8000b46
Gerrit-Change-Number: 18379
Gerrit-PatchSet: 1
Gerrit-Owner: Friendly Automation
Gerrit-CC: Benjamin Keith Ford <bford at digium.com>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20220414/9bae5456/attachment-0001.html>
More information about the asterisk-code-review
mailing list