Skip to content

Commit ac8147a

Browse files
committed
use sshbuf_allocate() to pre-allocate the buffer used for loading
keys. This avoids implicit realloc inside the buffer code, which might theoretically leave fragments of the key on the heap. This doesn't appear to happen in practice for normal sized keys, but was observed for novelty oversize ones. Pointed out by Jann Horn of Project Zero; ok markus@
1 parent 66d9cec commit ac8147a

File tree

1 file changed

+14
-2
lines changed

1 file changed

+14
-2
lines changed

Diff for: usr.bin/ssh/authfile.c

+14-2
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
/* $OpenBSD: authfile.c,v 1.121 2016/04/09 12:39:30 djm Exp $ */
1+
/* $OpenBSD: authfile.c,v 1.122 2016/11/25 23:24:45 djm Exp $ */
22
/*
33
* Copyright (c) 2000, 2013 Markus Friedl. All rights reserved.
44
*
@@ -98,13 +98,25 @@ sshkey_load_file(int fd, struct sshbuf *blob)
9898
u_char buf[1024];
9999
size_t len;
100100
struct stat st;
101-
int r;
101+
int r, dontmax = 0;
102102

103103
if (fstat(fd, &st) < 0)
104104
return SSH_ERR_SYSTEM_ERROR;
105105
if ((st.st_mode & (S_IFSOCK|S_IFCHR|S_IFIFO)) == 0 &&
106106
st.st_size > MAX_KEY_FILE_SIZE)
107107
return SSH_ERR_INVALID_FORMAT;
108+
/*
109+
* Pre-allocate the buffer used for the key contents and clamp its
110+
* maximum size. This ensures that key contents are never leaked via
111+
* implicit realloc() in the sshbuf code.
112+
*/
113+
if ((st.st_mode & S_IFREG) == 0 || st.st_size <= 0) {
114+
st.st_size = 64*1024; /* 64k should be enough for anyone :) */
115+
dontmax = 1;
116+
}
117+
if ((r = sshbuf_allocate(blob, st.st_size)) != 0 ||
118+
(dontmax && (r = sshbuf_set_max_size(blob, st.st_size)) != 0))
119+
return r;
108120
for (;;) {
109121
if ((len = atomicio(read, fd, buf, sizeof(buf))) == 0) {
110122
if (errno == EPIPE)

0 commit comments

Comments
 (0)