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.

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

Add support for BIO_FP_TEXT in file BIOs #1153

Merged
merged 30 commits into from
Oct 18, 2023

Conversation

WillChilds-Klein
Copy link
Contributor

@WillChilds-Klein WillChilds-Klein commented Aug 14, 2023

Issues

Addresses CryptoAlg-2036

Notes

OpenSSL defines a "close flag" BIO_FP_TEXT which is used by callers to indicate that file-backed BIOs should be opened in "text" mode as opposed to the default of raw "binary" mode. Of platforms currently supported by AWS-LC, this is only meaningful on windows. From linux's fopen man page:

        The mode string can also include the letter 'b' either as a last
        character or as a character between the characters in any of the
        two-character strings described above.  This is strictly for
        compatibility with ISO C and has no effect; the 'b' is ignored on
        all POSIX conforming systems, including Linux.  (Other systems
        may treat text files and binary files differently, and adding the
        'b' may be a good idea if you do I/O to a binary file and expect
        that your program may be ported to non-UNIX environments.)

And from Windows:

t     Open in text (translated) mode. Carriage return-line feed (CR-LF)
      combinations are translated into single line feeds (LF) on input and LF
      characters are translated to CR-LF combinations on output. Also, CTRL+Z is
      interpreted as an end-of-file character on input.
b     Open in binary (untranslated) mode; translations involving
      carriage-return and line feed characters are suppressed.

The terminology here is a bit confusing. In text mode, "Input" here refers to reading files and "output" refers to writing files to disk. So, if an application is using conventional unixy LF line endings (\n) and then writes a line of text to a file, the Windows CRT will instead write a CRLF line ending (\r\n). When reading from a file, the CRT will translate CRLF back to LF transparently to the application. In binary mode, what you see is what you get and no translation occurs.

OpenSSL supports setting text mode for a variety of BIO operations (including when opening files with fopen), but only sets this flag internally when calling BIO_new_fp. Note that BIO_new_fp operates on open FILE *s, and does not itself open the file. Instead, it uses the Windows CRT-specific function _setmode to set binary/text mode on the already opened file.

To implement this functionality in AWS-LC, we only set translation mode in functions using the BIO_C_SET_FILE_PTR control directive: BIO_set_fp and BIO_new_fp. AWS-LC's BIO functions that call fopen along the way do not provide the caller a parameter for specifying flags, so we're limited here by the interface.

Finally, it's a little odd to refer to BIO_FP_TEXT as a "close flag", as the behavior it determines has nothing to do with how or if the file is closed. However, this is how OpenSSL's code refers to it, so it's the terminology we use here.

Links

Documentation links for the Windows-specific functions used in this change can be found here:

  • _setmode: used to set translation mode on an open file
  • _fileno: used to convert a FILE * to a file descriptor

Testing

  • CI runs with new platform-aware test

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

@WillChilds-Klein WillChilds-Klein force-pushed the bio-fp-text branch 2 times, most recently from e24b35e to 6a21c52 Compare August 15, 2023 15:03
@WillChilds-Klein WillChilds-Klein marked this pull request as ready for review August 16, 2023 15:09
@WillChilds-Klein WillChilds-Klein requested a review from a team as a code owner August 16, 2023 15:09
@WillChilds-Klein WillChilds-Klein enabled auto-merge (squash) August 16, 2023 15:15
@WillChilds-Klein WillChilds-Klein force-pushed the bio-fp-text branch 4 times, most recently from 2938856 to 1e31cbc Compare August 21, 2023 18:59
@dkostic dkostic requested a review from justsmth August 22, 2023 17:13
@WillChilds-Klein WillChilds-Klein force-pushed the bio-fp-text branch 2 times, most recently from f3d548a to 1fd9b9f Compare August 28, 2023 15:45
@WillChilds-Klein WillChilds-Klein force-pushed the bio-fp-text branch 3 times, most recently from 1435270 to 980db0a Compare September 6, 2023 18:55
@WillChilds-Klein WillChilds-Klein force-pushed the bio-fp-text branch 4 times, most recently from 11c8707 to d244db5 Compare September 11, 2023 19:01
justsmth
justsmth previously approved these changes Sep 11, 2023
Copy link
Contributor

@justsmth justsmth left a comment

Choose a reason for hiding this comment

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

Not part of this PR, but I noticed that BIO_get_fp also doesn't have much test coverage.

```
[----------] 1 test from BIOTest
[ RUN      ] BIOTest.TextFile
..\crypto\bio\bio_test.cc(200): error: Expected equality of these values:
  "test\n"
  std::string(contents)
    Which is: "test\r\r\n"
[  FAILED  ] BIOTest.TextFile (1 ms)
[----------] 1 test from BIOTest (1 ms total)
```
@WillChilds-Klein WillChilds-Klein merged commit 7285f2e into aws:main Oct 18, 2023
@WillChilds-Klein WillChilds-Klein deleted the bio-fp-text branch October 18, 2023 21:42
WillChilds-Klein added a commit that referenced this pull request Dec 13, 2024
Upstream commit [`5ee4e95`][1]'s source changes are almost identical to
our
addition of `BIO_FP_TEXT` in [PR 1153][2]. The only difference in
behavior is
that we will call `_setmode` w/ `_O_BINARY` on windows when no flags are
specified, where BoringSSL will simply no-op and leave the underlying
file's
mode alone.

Upstream's commit has some nice tests, though, so we crib those and
change a
single line of expectation to account for our difference in behavior.

[1]:
google/boringssl@5ee4e95
[2]: #1153
[3]:
https://github.com/openssl/openssl/blob/dc10ffc2834e0d2f5ebc1c3e29bd97f1f43a0ead/crypto/bio/bss_file.c#L235-L237
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants