yggdrasil/ring
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Don't consume the newline in BIO_gets for fds

Browse Source 
We support BIO_gets on three BIOs. They're all slightly different. File
BIOs have the NUL truncation bug. fd BIOs swallow the embedded newline.
This CL fixes the second issue and updates the BIO_gets test to cover
all three.

See also upstream's https://github.com/openssl/openssl/pull/3442

Update-Note: BIO_gets on an fd BIO now returns the newline, to align
with memory and file BIOs. BIO_gets is primarily used in the PEM parser,
which tries to tolerate both cases, but this reduces the risk of some
weird bug that only appears in fd BIOs.

Change-Id: Ia8ffb8c67b6981d6ef7144a1522f8605dc01d525
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58552
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This commit is contained in:
David Benjamin 2023-04-03 13:58:57 +09:00 committed by Boringssl LUCI CQ
parent 9a56503c15
commit 0c069cbf33
3 changed files with 68 additions and 28 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

71
crypto/bio/bio_test.cc
View File

@ -364,7 +364,7 @@ TEST(BIOTest, MemWritable) {
EXPECT_EQ(BIO_eof(bio.get()), 1);
}
TEST(BIOTest, MemGets) {
TEST(BIOTest, Gets) {
const struct {
std::string bio;
int gets_len;
@ -386,8 +386,6 @@ TEST(BIOTest, MemGets) {
{"12345", 10, "12345"},
// NUL bytes do not terminate gets.
// TODO(davidben): File BIOs don't get this right. It's unclear if it's
// even possible to use fgets correctly here.
{std::string("abc\0def\nghi", 11), 256, std::string("abc\0def\n", 8)},
// An output size of one means we cannot read any bytes. Only the trailing
@ -403,22 +401,61 @@ TEST(BIOTest, MemGets) {
SCOPED_TRACE(t.bio);
SCOPED_TRACE(t.gets_len);
bssl::UniquePtr<BIO> bio(BIO_new_mem_buf(t.bio.data(), t.bio.size()));
ASSERT_TRUE(bio);
auto check_bio_gets = [&](BIO *bio) {
std::vector<char> buf(t.gets_len, 'a');
int ret = BIO_gets(bio, buf.data(), t.gets_len);
ASSERT_GE(ret, 0);
// |BIO_gets| should write a NUL terminator, not counted in the return
// value.
EXPECT_EQ(Bytes(buf.data(), ret + 1),
Bytes(t.gets_result.data(), t.gets_result.size() + 1));
std::vector<char> buf(t.gets_len, 'a');
int ret = BIO_gets(bio.get(), buf.data(), t.gets_len);
ASSERT_GE(ret, 0);
// |BIO_gets| should write a NUL terminator, not counted in the return
// value.
EXPECT_EQ(Bytes(buf.data(), ret + 1),
Bytes(t.gets_result.data(), t.gets_result.size() + 1));
// The remaining data should still be in the BIO.
buf.resize(t.bio.size() + 1);
ret = BIO_read(bio, buf.data(), static_cast<int>(buf.size()));
ASSERT_GE(ret, 0);
EXPECT_EQ(Bytes(buf.data(), ret),
Bytes(t.bio.substr(t.gets_result.size())));
};
// The remaining data should still be in the BIO.
const uint8_t *contents;
size_t len;
ASSERT_TRUE(BIO_mem_contents(bio.get(), &contents, &len));
EXPECT_EQ(Bytes(contents, len), Bytes(t.bio.substr(ret)));
{
SCOPED_TRACE("memory");
bssl::UniquePtr<BIO> bio(BIO_new_mem_buf(t.bio.data(), t.bio.size()));
ASSERT_TRUE(bio);
check_bio_gets(bio.get());
}
using ScopedFILE = std::unique_ptr<FILE, decltype(&fclose)>;
ScopedFILE file(tmpfile(), fclose);
ASSERT_TRUE(file);
if (!t.bio.empty()) {
ASSERT_EQ(1u,
fwrite(t.bio.data(), t.bio.size(), /*nitems=*/1, file.get()));
ASSERT_EQ(0, fseek(file.get(), 0, SEEK_SET));
}
// TODO(crbug.com/boringssl/585): If the line has an embedded NUL, file
// BIOs do not currently report the answer correctly.
if (t.bio.find('\0') == std::string::npos) {
SCOPED_TRACE("file");
bssl::UniquePtr<BIO> bio(BIO_new_fp(file.get(), BIO_NOCLOSE));
ASSERT_TRUE(bio);
check_bio_gets(bio.get());
}
ASSERT_EQ(0, fseek(file.get(), 0, SEEK_SET));
{
SCOPED_TRACE("fd");
#if defined(OPENSSL_WINDOWS)
int fd = _fileno(file.get());
#else
int fd = fileno(file.get());
#endif
bssl::UniquePtr<BIO> bio(BIO_new_fd(fd, BIO_NOCLOSE));
ASSERT_TRUE(bio);
check_bio_gets(bio.get());
}
}
// Negative and zero lengths should not output anything, even a trailing NUL.

11
crypto/bio/fd.c
View File

@ -241,15 +241,18 @@ static long fd_ctrl(BIO *b, int cmd, long num, void *ptr) {
}
static int fd_gets(BIO *bp, char *buf, int size) {
char *ptr = buf;
char *end = buf + size - 1;
if (size <= 0) {
return 0;
}
while (ptr < end && fd_read(bp, ptr, 1) > 0 && ptr[0] != '\n') {
char *ptr = buf;
char *end = buf + size - 1;
while (ptr < end && fd_read(bp, ptr, 1) > 0) {
char c = ptr[0];
ptr++;
if (c == '\n') {
break;
}
}
ptr[0] = '\0';

14
include/openssl/bio.h
View File

@ -107,14 +107,14 @@ OPENSSL_EXPORT int BIO_up_ref(BIO *bio);
// bytes read, zero on EOF, or a negative number on error.
OPENSSL_EXPORT int BIO_read(BIO *bio, void *data, int len);
// BIO_gets "reads a line" from |bio| and puts at most |size| bytes into |buf|.
// It returns the number of bytes read or a negative number on error. The
// phrase "reads a line" is in quotes in the previous sentence because the
// exact operation depends on the BIO's method. For example, a digest BIO will
// return the digest in response to a |BIO_gets| call.
// BIO_gets reads a line from |bio| and writes at most |size| bytes into |buf|.
// It returns the number of bytes read or a negative number on error. This
// function's output always includes a trailing NUL byte, so it will read at
// most |size - 1| bytes.
//
// TODO(fork): audit the set of BIOs that we end up needing. If all actually
// return a line for this call, remove the warning above.
// If the function read a complete line, the output will include the newline
// character, '\n'. If no newline was found before |size - 1| bytes or EOF, it
// outputs the bytes which were available.
OPENSSL_EXPORT int BIO_gets(BIO *bio, char *buf, int size);
// BIO_write writes |len| bytes from |data| to |bio|. It returns the number of