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

pppd: add error message getter for openssl #508

Closed
wants to merge 1 commit into from
Closed

pppd: add error message getter for openssl #508

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 17 additions & 0 deletions pppd/crypto.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@

#ifdef PPP_WITH_OPENSSL
#include <openssl/opensslv.h>
#include <openssl/err.h>
#endif

#if OPENSSL_VERSION_NUMBER >= 0x30000000L
Expand Down Expand Up @@ -179,6 +180,22 @@ int PPP_crypto_deinit()
return 1;
}

char *PPP_crypto_get_error()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will work, and generally I think it can be committed with further ado. However, you could consider

  1. Avoid multi-line errors and maybe get the last error at the top of the stack?
  2. Change this to a function signature such that it takes a format string, adds the error information and calls error().. e.g. PPP_ERR_printf("chap failed, md4 initialization failed");
  3. All these heap allocations bugs me when memory may not need to be allocated, could consider making err_buf[256] a part of PPP_MD_CTX or PPP_CHIPHER_CTX and then use that with an snprintf. Yes, an imposed limit isn't ideal, but it would prevent it from spewing out multi-line information in the logs.

In the secondary example, we could for every if-else situation where the crypto api is called add the code to improve current status quo, instead of just the init functions. If we make it a part of the shim API, then the caller won't need to add the if-else, but you loose any contextual information such as e.g. ms-chap-crypto failed, md4 algorithm couldn't be initialized.

Yes, openssl has a similar get_error() and get_error_string() functions, and for this to work you also need to load the strings, e.g. SSL_load_error_strings, unless the BIO api doesn't require this.

To make this a complete change, perhaps don't do the error handling in the individual md4/md5/sha1 files, but move it one level up to the PPP_DigestInit(), PPP_CipherInit() and friends.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for your comments, if I understand correctly

  • change mutli line by last error
  • change the error API to get a format string as input to prepend to openssl error
  • avoid heap allocations (this one bugs me since I will not know the size of error string in advance and it may be cut by the snprintf that we will do)
  • move the error management in the PPP crypto abstraction layer.

I'll work on that as soon as I have free time

{
char* ret = NULL;
#ifdef PPP_WITH_OPENSSL
BIO *bio = BIO_new (BIO_s_mem ());
ERR_print_errors (bio);
char *buf = NULL;
size_t len = BIO_get_mem_data (bio, &buf);
ret = (char *) calloc (1, 1 + len);
if (ret)
memcpy (ret, buf, len);
BIO_free (bio);
#endif
return ret;
}

#ifdef UNIT_TEST
#include <stdio.h>

Expand Down
7 changes: 7 additions & 0 deletions pppd/crypto.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,13 @@ int PPP_crypto_init();
*/
int PPP_crypto_deinit();

/*
* Get possible human readable error message from crypto
* return string must be freed unless NULL (which is what
* is returned if compilation is done without openssl)
*/
char *PPP_crypto_get_error();

#ifdef __cplusplus
}
#endif
Expand Down
7 changes: 7 additions & 0 deletions pppd/ppp-md4.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
#define EVP_MD_CTX_new EVP_MD_CTX_create
#endif

#include "pppd-private.h"

static int md4_init(PPP_MD_CTX *ctx)
{
Expand All @@ -55,6 +56,12 @@ static int md4_init(PPP_MD_CTX *ctx)
if (EVP_DigestInit(mctx, EVP_md4())) {
ctx->priv = mctx;
return 1;
} else {
char* err = PPP_crypto_get_error();
if (err) {
error("EVP_DigestInit failed: %s", err);
free(err);
}
}
EVP_MD_CTX_free(mctx);
}
Expand Down
8 changes: 8 additions & 0 deletions pppd/ppp-md5.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@
#define EVP_MD_CTX_new EVP_MD_CTX_create
#endif

#include "pppd-private.h"

static int md5_init(PPP_MD_CTX *ctx)
{
if (ctx) {
Expand All @@ -54,6 +56,12 @@ static int md5_init(PPP_MD_CTX *ctx)
if (EVP_DigestInit((EVP_MD_CTX*) mctx, EVP_md5())) {
ctx->priv = mctx;
return 1;
} else {
char* err = PPP_crypto_get_error();
if (err) {
error("EVP_DigestInit failed: %s", err);
free(err);
}
}
EVP_MD_CTX_free(mctx);
}
Expand Down
8 changes: 8 additions & 0 deletions pppd/ppp-sha1.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@
#define EVP_MD_CTX_new EVP_MD_CTX_create
#endif

#include "pppd-private.h"

static int sha1_init(PPP_MD_CTX *ctx)
{
if (ctx) {
Expand All @@ -56,6 +58,12 @@ static int sha1_init(PPP_MD_CTX *ctx)
if (EVP_DigestInit(mctx, EVP_sha1())) {
ctx->priv = mctx;
return 1;
} else {
char* err = PPP_crypto_get_error();
if (err) {
error("EVP_DigestInit failed: %s", err);
free(err);
}
}
EVP_MD_CTX_free(mctx);
}
Expand Down