@@ -93,6 +93,7 @@ int usign_s(const char *msgfile, const char *seckeyfile, const char *sigfile, bo
*/
static int usign_f(char fingerprint[17], const char *pubkeyfile, const char *seckeyfile, const char *sigfile, bool quiet) {
int fds[2];
+ FILE *f;
pid_t pid;
int status;
const char *usign_argv[16] = {0};
@@ -141,17 +142,22 @@ static int usign_f(char fingerprint[17], const char *pubkeyfile, const char *sec
waitpid(pid, &status, 0);
status = WIFEXITED(status) ? WEXITSTATUS(status) : -1;
- if (fingerprint && !status) {
- ssize_t r;
- memset(fingerprint, 0, 17);
- r = read(fds[0], fingerprint, 17);
- if (r < 16)
- status = -1;
+ if (!fingerprint || status) {
+ close(fds[0]);
+ return status;
+ }
- fingerprint[16] = '\0';
+ f = fdopen(fds[0], "r");
+ if (fread(fingerprint, 1, 16, f) != 16)
+ status = -1;
+ fclose(f);
+ if (status)
+ return status;
+
+ fingerprint[16] = '\0';
+ if (strspn(fingerprint, "0123456789abcdefABCDEF") != 16)
+ status = -1;
- }
- close(fds[0]);
return status;
}
While not likely to happen in pratice, nothing guarantees that read() will retrieve more than 1 byte at a time. The easiest way to make this code compliant is to wrap the file descriptor using fdopen(). While we're at it, also - remove useless memset() - check fingerprint for validity The check is particularly relevant, as a usign bug [1] causing short fingerprint outputs only went unnoticed for so long because the trailing newline was considered one of the 16 characters ucert was expecting. [1] https://patchwork.ozlabs.org/project/openwrt/patch/8ead1fd6a61117b54b4efd5111fe0d19e4eef9c5.1589642591.git.mschiffer@universe-factory.net/ Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net> --- usign-exec.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-)