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.

Sign up for GitHub

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

Handle errors when signing SSH certificates #422

Merged
merged 9 commits into from
Jan 10, 2025
Merged

Handle errors when signing SSH certificates #422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
22feb84
Handle errors when signing SSH certificates
aveenismail Sep 3, 2024
b63d963
Remove unnecessary < 0 check
aveenismail Sep 4, 2024
a4ebbf9
Add length check of ED key
aveenismail Sep 4, 2024
48df203
Calculate lengths instead of using magic numbers
aveenismail Sep 4, 2024
249a87f
Revert unnecessary changes
aveenismail Sep 4, 2024
4ad3333
Free memory if BIO_write() fails
aveenismail Sep 4, 2024
0657a90
Code optimization
aveenismail Sep 5, 2024
d0aba38
Remove unnecessary return value check
aveenismail Jan 10, 2025
d560263
Remove unecessary void castings
aveenismail Jan 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 37 additions & 14 deletions common/util.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,9 @@ bool read_ed25519_key(uint8_t *in, size_t in_len, uint8_t *out,

uint8_t decoded[128];
size_t decoded_len = sizeof(decoded);

if (in_len < (28 + 26)) {
return false;
}

if (memcmp(in, PEM_private_header, 28) != 0 ||
memcmp(in + in_len - 26, PEM_private_trailer, 25) != 0) {
return false;
Expand All @@ -95,13 +93,19 @@ bool read_ed25519_key(uint8_t *in, size_t in_len, uint8_t *out,
BIO_set_flags(b64, BIO_FLAGS_BASE64_NO_NL);
BIO_push(b64, bio);

(void) BIO_write(bio, in + 28, in_len - 28 - 25);
(void) BIO_flush(bio);
if (BIO_write(bio, in + 28, in_len - 28 - 25) <= 0) {
BIO_free_all(b64);
return false;
}
if(BIO_flush(bio) != 1) {
BIO_free_all(b64);
return false;
}
ret = BIO_read(b64, decoded, decoded_len);

BIO_free_all(b64);

if (ret <= 0 || ret != 48) {
if (ret != 48) {
return false;
}

Expand Down Expand Up @@ -171,7 +175,10 @@ bool read_private_key(uint8_t *buf, size_t len, yh_algorithm *algo,
return false;
}

(void) BIO_write(bio, buf, len);
if(BIO_write(bio, buf, len) <= 0) {
BIO_free_all(bio);
return false;
}

private_key = PEM_read_bio_PrivateKey(bio, NULL, NULL, /*password*/ NULL);
BIO_free_all(bio);
Expand Down Expand Up @@ -376,7 +383,10 @@ bool read_public_key(uint8_t *buf, size_t len, yh_algorithm *algo,
return false;
}

(void) BIO_write(bio, buf, len);
if(BIO_write(bio, buf, len) <= 0) {
BIO_free_all(bio);
return false;
}

EVP_PKEY *pubkey = PEM_read_bio_PUBKEY(bio, NULL, NULL, NULL);
BIO_free_all(bio);
Expand Down Expand Up @@ -430,7 +440,7 @@ bool read_public_key(uint8_t *buf, size_t len, yh_algorithm *algo,
return false;
}

(void) i2o_ECPublicKey(ec, &bytes);
i2o_ECPublicKey(ec, &bytes);
EC_KEY_free(ec);

*bytes_len = data_len;
Expand Down Expand Up @@ -643,8 +653,14 @@ bool base64_decode(const char *in, uint8_t *out, size_t *len) {
BIO_set_flags(b64, BIO_FLAGS_BASE64_NO_NL);
BIO_push(b64, bio);

(void) BIO_write(bio, in, strlen(in));
(void) BIO_flush(bio);
if(BIO_write(bio, in, strlen(in)) <= 0) {
BIO_free_all(b64);
return false;
}
if(BIO_flush(bio) != 1) {
BIO_free_all(b64);
return false;
}
ret = BIO_read(b64, out, *len);

BIO_free_all(b64);
Expand Down Expand Up @@ -680,10 +696,17 @@ bool write_file(const uint8_t *buf, size_t buf_len, FILE *fp, format_t format) {
}
bio = BIO_push(b64, bio);

(void) BIO_set_flags(bio, BIO_FLAGS_BASE64_NO_NL);
(void) BIO_write(bio, buf, buf_len);
(void) BIO_flush(bio);
(void) BIO_get_mem_ptr(bio, &bufferPtr);
BIO_set_flags(bio, BIO_FLAGS_BASE64_NO_NL);
if(BIO_write(bio, buf, buf_len) <= 0) {
BIO_free_all(bio);
return false;
}
if(BIO_flush(bio) != 1) {
BIO_free_all(bio);
return false;
}
BIO_get_mem_ptr(bio, &bufferPtr);

p = (uint8_t *) bufferPtr->data;
length = bufferPtr->length;
} else if (format == _hex) {
Expand Down
8 changes: 4 additions & 4 deletions examples/ssh.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,10 +280,10 @@ int main(void) {
bio = BIO_push(b64, bio);

fprintf(stdout, "ssh-rsa-cert-v01@openssh.com ");
(void) BIO_set_flags(bio, BIO_FLAGS_BASE64_NO_NL);
(void) BIO_write(bio, ssh_req + 4 + 256,
ssh_req_len + ssh_cert_len - 4 - 256);
(void) BIO_flush(bio);
BIO_set_flags(bio, BIO_FLAGS_BASE64_NO_NL);
assert(BIO_write(bio, ssh_req + 4 + 256,
ssh_req_len + ssh_cert_len - 4 - 256) > 0);
assert(BIO_flush(bio) == 1);
fprintf(stdout, "\n");

BIO_free_all(bio);
Expand Down
4 changes: 3 additions & 1 deletion lib/yubihsm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ uint8_t _yh_verbosity YH_INTERNAL = 0;
FILE *_yh_output YH_INTERNAL = NULL;
#endif

#define UNUSED(x) (void) (x);

static yh_rc compute_full_mac_ex(const uint8_t *data, uint16_t data_len,
aes_context *aes_ctx, uint8_t *mac) {

Expand Down Expand Up @@ -4666,7 +4668,7 @@ yh_rc yh_init(void) {
static yh_rc load_backend(const char *name,
void **backend,
struct backend_functions **bf) {
(void)backend;
UNUSED(backend);
if (name == NULL) {
DBG_ERR("No name given to load_backend");
return YHR_GENERIC_ERROR;
Expand Down
12 changes: 7 additions & 5 deletions lib/yubihsm_usb.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
#include "yubihsm_usb.h"
#include "debug_lib.h"

#define UNUSED(x) (void) (x)

#ifndef STATIC
uint8_t YH_INTERNAL _yh_verbosity;
FILE YH_INTERNAL *_yh_output;
Expand All @@ -51,7 +53,7 @@ static yh_rc backend_connect(yh_connector *connector, int timeout) {
yh_rc ret = YHR_CONNECTOR_ERROR;
yh_backend *backend = NULL;

(void) timeout;
UNUSED(timeout);

if (parse_usb_url(connector->api_url, &serial) == false) {
DBG_ERR("Failed to parse URL: '%s'", connector->api_url);
Expand Down Expand Up @@ -82,7 +84,7 @@ static yh_rc backend_send_msg(yh_backend *connection, Msg *msg, Msg *response,
yh_rc ret = YHR_GENERIC_ERROR;
unsigned long read_len = 0;

(void) identifier;
UNUSED(identifier);

for (int i = 0; i <= 1; i++) {
if (ret != YHR_GENERIC_ERROR) {
Expand Down Expand Up @@ -131,9 +133,9 @@ static void backend_cleanup(void) { DBG_INFO("backend_cleanup"); }

static yh_rc backend_option(yh_backend *connection, yh_connector_option opt,
const void *val) {
(void) connection;
(void) opt;
(void) val;
UNUSED(connection);
UNUSED(opt);
UNUSED(val);

DBG_ERR("Backend options not (yet?) supported for USB");
return YHR_CONNECTOR_ERROR;
Expand Down
6 changes: 3 additions & 3 deletions lib/yubihsm_winhttp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,9 +297,9 @@ static yh_rc backend_send_msg(yh_backend *backend, Msg *msg, Msg *response,

static yh_rc backend_option(yh_backend *connection, yh_connector_option opt,
const void *val) {
(void) connection;
(void) opt;
(void) val;
UNUSED(connection);
UNUSED(opt);
UNUSED(val);

DBG_ERR("Backend options not (yet?) supported with winhttp");
return YHR_CONNECTOR_ERROR;
Expand Down
2 changes: 1 addition & 1 deletion pkcs11/util_pkcs11.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3813,7 +3813,7 @@ void verify_mechanism_cleanup(yubihsm_pkcs11_op_info *op_info) {

void decrypt_mechanism_cleanup(yubihsm_pkcs11_op_info *op_info) {

(void) op_info;
UNUSED(op_info);
}

void digest_mechanism_cleanup(yubihsm_pkcs11_op_info *op_info) {
Expand Down
55 changes: 38 additions & 17 deletions src/commands.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ int yh_com_connect(yubihsm_context *ctx, Argument *argv, cmd_format in_fmt,
}
yrc = yh_connect(ctx->connector, 0);
if (yrc == YHR_SUCCESS) {
(void) yh_com_keepalive_on(NULL, NULL, fmt_nofmt, fmt_nofmt);
yh_com_keepalive_on(NULL, NULL, fmt_nofmt, fmt_nofmt);
return 0;
}
fprintf(stderr, "Failed connecting '%s': %s\n", ctx->connector_list[i],
Expand Down Expand Up @@ -1158,18 +1158,21 @@ int yh_com_get_pubkey(yubihsm_context *ctx, Argument *argv, cmd_format in_fmt,
BIO *bio = BIO_new_fp(ctx->out, BIO_NOCLOSE);
if (bio == NULL) {
fprintf(stderr, "Unable to allocate BIO\n");
BIO_free_all(b64);
error = true;
goto getpk_base64_cleanup;
}

bio = BIO_push(b64, bio);

(void) i2d_PUBKEY_bio(bio, public_key);
i2d_PUBKEY_bio(bio, public_key);

(void) BIO_flush(bio);
(void) BIO_free_all(bio);
if (BIO_flush(bio) != 1) {
fprintf(stderr, "Unable to flush BIO\n");
error = true;
goto getpk_base64_cleanup;
}
getpk_base64_cleanup:
BIO_free_all(b64);
if (error) {
EVP_PKEY_free(public_key);
return -1;
Expand Down Expand Up @@ -1258,10 +1261,15 @@ int yh_com_get_device_pubkey(yubihsm_context *ctx, Argument *argv,

bio = BIO_push(b64, bio);

(void) i2d_PUBKEY_bio(bio, public_key);
i2d_PUBKEY_bio(bio, public_key);

(void) BIO_flush(bio);
(void) BIO_free_all(bio);
if (BIO_flush(bio) != 1) {
fprintf(stderr, "Unable to flush BIO\n");
BIO_free_all(b64);
error = true;
goto getdpk_base64_cleanup;
}
BIO_free_all(bio);
getdpk_base64_cleanup:
if (error) {
EVP_PKEY_free(public_key);
Expand Down Expand Up @@ -2467,7 +2475,11 @@ static bool read_rsa_pubkey(const uint8_t *buf, size_t len,
if ((bio = BIO_new(BIO_s_mem())) == NULL)
return false;

(void) BIO_write(bio, buf, len);
if(BIO_write(bio, buf, len) <= 0) {
fprintf(stderr, "%s: Failed to read RSA public key\n", __func__);
BIO_free_all(bio);
return false;
}

RSA *rsa = NULL;
EVP_PKEY *pubkey = PEM_read_bio_PUBKEY(bio, NULL, NULL, NULL);
Expand Down Expand Up @@ -3136,15 +3148,21 @@ int yh_com_sign_ssh_certificate(yubihsm_context *ctx, Argument *argv,
}
bio = BIO_push(b64, bio);

int ret = 0;
int cert_len = argv[4].len - certdata_offset + response_len;
BUF_MEM *bufferPtr = 0;

BIO_set_flags(bio, BIO_FLAGS_BASE64_NO_NL);
if (BIO_write(bio, data + certdata_offset, cert_len) != cert_len) {
fprintf(stderr, "Failed to write SSH certificate.\n");
return -1;
fprintf(stderr, "Failed to write SSH certificate.\n");
ret = -1;
goto clean_bio;
}
if (BIO_flush(bio) != 1) {
fprintf(stderr, "Failed to sign SSH certificate.\n");
ret = -1;
goto clean_bio;
}
BIO_flush(bio);
BIO_get_mem_ptr(bio, &bufferPtr);

const char *ssh_cert_str =
Expand All @@ -3154,24 +3172,27 @@ int yh_com_sign_ssh_certificate(yubihsm_context *ctx, Argument *argv,
strlen(ssh_cert_str) ||
ferror(ctx->out)) {
fprintf(stderr, "Unable to write data to file\n");
return -1;
ret = -1;
goto clean_bio;
}

if (fwrite(bufferPtr->data, 1, bufferPtr->length, ctx->out) !=
bufferPtr->length ||
ferror(ctx->out)) {
fprintf(stderr, "Unable to write data to file\n");
return -1;
ret = -1;
goto clean_bio;
}

if (fwrite("\n", 1, 1, ctx->out) != 1 || ferror(ctx->out)) {
fprintf(stderr, "Unable to write data to file\n");
return -1;
ret = -1;
}

(void) BIO_free_all(bio); // TODO: fix this leak.
clean_bio:
BIO_free_all(bio);

return 0;
return ret;
}

static void time_elapsed(struct timeval *after, struct timeval *before,
Expand Down
Loading