Regression in backported patch for openssl 1.1

Bug #1835968 reported by Unit 193 on 2019-07-09
14
This bug affects 1 person
Affects Status Importance Assigned to Milestone
ruby2.5 (Ubuntu)
High
Unassigned
Bionic
High
Bryce Harrington

Bug Description

[Impact]
Transfer of certain files will fail with an error, which had been
transferring properly prior to ruby2.5 (2.5.1-1ubuntu1.4), which
introduced improved openssl 1.1 support.

[Test Case]
  $ lxc create ubuntu:18.04/amd64 ruby25-sru-1835968-bionic
  $ lxc exec ruby25-sru-1835968-bionic -- bash
  (...)
  # sudo apt-get update
  $ sudo apt-get -y install ruby-roadie ruby

  # Preconfigure as Internet site
  $ sudo debconf-set-selections <<< "postfix postfix/mailname string ruby25-sru-1835968-bionic.lxd"
  $ sudo debconf-set-selections <<< "postfix postfix/main_mailer_type string 'Internet Site'"
  $ sudo apt-get install -y postfix

  ./testcase-ruby-ssl.rb (private)

  With the current Ubuntu version, the testcase generates a backtrace
  ending in an error message involving ASCII/UTF8 confusion.

  With the PPA version, the test file gets successfully delivered to a
  local mailbox

[Regression Potential]
This code change affects the processing of buffer data sent via network
when using openssl. Things to watch for especially are misbehaviors
with line separation handling, particularly with files containing mixed
CRLF / LF newlines.

The fix changes a patch that provides string optimization in network
transfers, so another thing to watch for would be severe increase in
memory utilization or decrease in throughput when doing heavy file
operations over the network.

[Fix]
Fix restores bit of code that had been dropped by a recent patch,
introducing a regression when transferring certain files using openssh.

PPA with test package: https://launchpad.net/~bryce/+archive/ubuntu/ruby2.5-sru-1835968
  $ sudo add-apt-repository ppa:bryce/ruby2.5-sru-1835968
  $ sudo apt-get update
  $ sudo apt-get install ruby2.5

[Discussion]
The regression was seen after updating to the current version of
ruby2.5. Examining the patches introduced in that release identified
0009-test-test_pair-fix-deadlock-in-test_connect_accept_n.patch as the
source of the problem. This patch is a collection of 17 commits
collected as a sync from the ruby-openssl upstream project into
ruby2.5's openssl extension library. Running the testcase against these
patches identified the 17th one in the series as the problematic patch
(SHA 251b5be2), and incremental testing of each discrete change within
that commit identified a specific change to an if statement's
conditionals that triggered the failure. See LP: #1835968's commentary
for more detailed discussion.

Restoring the original version of this conditional shouldn't adversely
affect the optimization provided in SHA 251b5be2.

[Original Report]

Howdy,

After the update of ruby2.5 (2.5.1-1ubuntu1.4) I started getting this error "incompatible character encodings: ASCII-8BIT and UTF-8" intermittently, depending on contents. I copied /usr/lib/ruby/2.5.0/openssl/buffering.rb (as mentioned in the root of the traceback) over from buster/testing and the issues went away.

When using 'file' to check the output documents for what failed in Ubuntu vs the ones that didn't, I got the following results:

Works: "HTML document, UTF-8 Unicode text, with very long lines"
Fails: "HTML document, UTF-8 Unicode text, with very long lines, with CRLF, LF line terminators"

The diff of buffering.rb between Ubuntu and Debian is attached.

Related branches

Unit 193 (unit193) wrote :
Changed in ruby2.5 (Ubuntu):
importance: Undecided → High
tags: added: regression-update
tags: added: rls-bb-incoming
Bryce Harrington (bryce) wrote :

Thanks for flagging this.

Version 2.5.1-1ubuntu1.4 matches to what's currently carried in 18.04 so am guessing you're on that release. What ruby2.5 version did you upgrade from? (Check your /var/log/apt/*.log files.)

Also, please provide a minimal test case to reproduce the bug. I.e. something like:

"""
$ apt-get -y install ruby2.5
$ ruby
print "Hello world"
<some code>

^D
Error: incompatible character encodings: ASCII-8BIT and UTF-8
"""

Changed in ruby2.5 (Ubuntu):
status: New → Incomplete
Bryce Harrington (bryce) wrote :

Also, the title you selected for the bug, "Regression in backported patch for openssl 1.1" suggests you suspect this relates to the recent openssl 1.1.1 changes, however I'm unclear on how that would relate to encoding issues in ruby, so it would help if you could elaborate on your suspicions there.

Unit 193 (unit193) wrote :

Howdy,

My apologies I never mentioned what version I was on. And I went from 2.5.1-1ubuntu1.2 to 2.5.1-1ubuntu1.4 it would seem.

I don't exactly have a minimal testcase since the error happened when interacting with a smtpd.

As part of the openssl 1.1 upgrade, a couple patches were added to ruby2.5. If you note the path I mentioned it's the embedded copy of ruby/openssl that libruby2.5 ships. If you look at ruby2.5 now, Ubuntu eoan carries a couple patches over what Debian has. Given the package changelog, I would presume 3f6e30e53ce8050375955322e170612e1de099b1 is the offending commit[0].

The diff I attached shows that Ubuntu also added 251b5be20d5b58c27490f44cdeb6e655f9be6f19 over what Debian has, I could add that patch back to my system and see if I still am error free if that'd help in any way.

[0]: https://github.com/ruby/openssl/commit/3f6e30e53ce8050375955322e170612e1de099b1

~Unit 193

Unit 193 (unit193) wrote :

Howdy,

So I got a bit of a simpler testcase worked out (though due to reasons, it's not something I'd like to make fully public, can PM it) and it ended up being 251b5be20d5b58c27490f44cdeb6e655f9be6f19 that was giving me the trouble.

I also made an Eoan chroot and tried the test script, it worked like a charm so this is limited to Bionic. That is to be expected though since Eoan doesn't carry the commit.

~Unit 193

Changed in ruby2.5 (Ubuntu):
status: Incomplete → Confirmed

Thanks for the extra info Unit193.

The first assumed change [1] is in file debian/patches/0001-openssl-buffering.rb-no-RS-when-output.patch

But [2] is the offending commit according to your analysis so far. And I can confirm that it is applied in Bionic. That comes in debian/patches/0009-openssl-sync-with-upstream-repository.patch whch is a big squashed patch not only the content of [2].

Both of these d/patches came in due to 2.5.1-1ubuntu1.4:
  * Cherrypick ruby-openssl upstream commits to fix compat with OpenSSL
    1.1.1 LP: #1797386

I agree that this change isn't in Eoan (which in itself is a violation of the SRU policy I'd think). Neither is it in Cosmic/Disco. But maybe there is more special magic to it e.g. in those later versions not using that internal lib, I'll leave that to xnox.

There is no later on upstream fix to [2] in the repository yet, maybe it is ok there but needs some backporting to properly work with 2.5.1 ?

@Unit193 - to confirm - reverting just [2] (and no other components of this) fixes the issue for you?

@Unit193 - too bad the testcase is "private" testcase for a start send it to the emails registered at https://launchpad.net/~xnox and https://launchpad.net/~paelzer

@xnox - Subscribing xnox who did the update to consider this change (and also uploads for later versions?). Would reverting just [2] out of the big squashed patch #9 be an option for you?

[1]: https://github.com/ruby/openssl/commit/3f6e30e53ce8050375955322e170612e1de099b1
[2]: https://github.com/ruby/openssl/commit/251b5be20d5b58c27490f44cdeb6e655f9be6f19

Unit 193 (unit193) wrote :

Howdy,

"to confirm - reverting just [2] (and no other components of this) fixes the issue for you?" Yes, this is precisely what I am saying. Reverting just that makes everything work for me.

Eh, the "testcase" isn't really a proper testcase, but it was narrowed down enough for me to reproduce the error and confirm when specifically it was gone. Otherwise it's pretty poor.

Thanks for looking into this issue!

~Unit 193

I got the testcase (and forwarded to xnox/bryce as it is only to be shared confidently)

I got a auth error at first and modified it to use a local server that one could spawn in a container. But I still fail at:
/usr/lib/ruby/2.5.0/net/smtp.rb:539:in `initialize': no implicit conversion of Hash into String (TypeError)

Usage (so far):
$ apt install ruby-roadie ruby
$ apt install postfix
  (follow [1] as you need TLS for smtp)

sasl uses pam here, so have a user ubuntu:ubuntu for the test.

Then run:
./testcase-ruby-ssl.rb

[1]: https://help.ubuntu.com/community/Postfix

Bryce Harrington (bryce) on 2019-07-24
Changed in ruby2.5 (Ubuntu Bionic):
status: New → Triaged
importance: Undecided → High
Bryce Harrington (bryce) wrote :

Here is a broken-out set of the individual patches included in 0009-openssl-sync-with-upstream-repository. Perhaps just one of the patches in this set causes the problem, that we could revert/fix.

Changed in ruby2.5 (Ubuntu Bionic):
assignee: nobody → Bryce Harrington (bryce)
Bryce Harrington (bryce) wrote :

Doing a quick scan through the patches, these look interesting:
* 0013-test-test_x509name-change-script-encoding-to-ASCII-8.patch
* 0014-x509name-refactor-OpenSSL-X509-Name-to_s.patch
* 0015-x509name-fix-handling-of-X509_NAME_-oneline-print_ex.patch
* 0016-openssl-buffering.rb-no-RS-when-output.patch (***)
* 0017-Reduce-memory-allocation-when-writing-to-SSLSocket.patch

Patch 0016 is the one that changes the line ending behavior in buffering.rb and so would probably be the one to test first. 0015 and 0016 also change buffering.rb, while 0013 and 0014 involve utf/ascii conversions so could be involved in some fashion.

I don't know precisely what was required for supporting openssl 1.1, however the earlier patches in the series (particularly 0005, 0006 and 0007, and likely 0001, 0002, and 0004) look most pertinent for that, so I suspect if we were to revert one or more of 0013 to 0017, it may not adversely affect desired functionality although they do appear to fix legit issues and probably worth keeping.

(Fwiw, patches 0009, 0010, and 0012 appear to be MS Windows specific so might be irrelevant to Ubuntu. Patches 0008 and 0011 appear to just be refactoring, although not 100% sure. 0003 is just docs.)

Bryce Harrington (bryce) wrote :

Looking through the upstream repository for what landed after the aforementioned patches, there aren't further changes against buffering.rb. I'm not noticing any clear followup fixes to changes to patches in this patchset. So, it might be worth testing if this issue is also reproducible in the upstream ruby2.5 tree.

In eoan and disco, just spot checking individual files looks like the upstream changes are not present, so as Christian said in comment #6 this change appears to have landed in bionic without being applied to eoan or disco.

Bryce Harrington (bryce) wrote :

Running the test case I ran into the same error Christian did, mentioned in comment #8, but after hacking on the testcase a bit (I don't know Ruby, but studying the code in /usr/lib/ruby/2.5.0/net/smtp.rb gave enough clues) I was able to reproduce:

$ ./testcase-ruby-ssl.rb
1
2
3
4
5
6
6.1Traceback (most recent call last):
 26: from ./testcase-ruby-ssl.rb:46:in `<main>'
 25: from /usr/lib/ruby/2.5.0/net/smtp.rb:519:in `start'
 24: from ./testcase-ruby-ssl.rb:48:in `block in <main>'
 23: from /usr/lib/ruby/2.5.0/net/smtp.rb:659:in `send_message'
 22: from /usr/lib/ruby/2.5.0/net/smtp.rb:854:in `rcptto_list'
 21: from /usr/lib/ruby/2.5.0/net/smtp.rb:659:in `block in send_message'
 20: from /usr/lib/ruby/2.5.0/net/smtp.rb:898:in `data'
 19: from /usr/lib/ruby/2.5.0/net/smtp.rb:960:in `critical'
 18: from /usr/lib/ruby/2.5.0/net/smtp.rb:904:in `block in data'
 17: from /usr/lib/ruby/2.5.0/net/protocol.rb:304:in `write_message'
 16: from /usr/lib/ruby/2.5.0/net/protocol.rb:224:in `writing'
 15: from /usr/lib/ruby/2.5.0/net/protocol.rb:305:in `block in write_message'
 14: from /usr/lib/ruby/2.5.0/net/protocol.rb:339:in `using_each_crlf_line'
 13: from /usr/lib/ruby/2.5.0/net/protocol.rb:306:in `block (2 levels) in write_message'
 12: from /usr/lib/ruby/2.5.0/net/protocol.rb:291:in `write_message_0'
 11: from /usr/lib/ruby/2.5.0/net/protocol.rb:350:in `each_crlf_line'
 10: from /usr/lib/ruby/2.5.0/net/protocol.rb:360:in `buffer_filling'
  9: from /usr/lib/ruby/2.5.0/net/protocol.rb:360:in `step'
  8: from /usr/lib/ruby/2.5.0/net/protocol.rb:362:in `block in buffer_filling'
  7: from /usr/lib/ruby/2.5.0/net/protocol.rb:352:in `block in each_crlf_line'
  6: from /usr/lib/ruby/2.5.0/net/protocol.rb:292:in `block in write_message_0'
  5: from /usr/lib/ruby/2.5.0/net/protocol.rb:233:in `write0'
  4: from /usr/lib/ruby/2.5.0/openssl/buffering.rb:338:in `write'
  3: from /usr/lib/ruby/2.5.0/openssl/buffering.rb:338:in `inject'
  2: from /usr/lib/ruby/2.5.0/openssl/buffering.rb:338:in `each'
  1: from /usr/lib/ruby/2.5.0/openssl/buffering.rb:339:in `block in write'
/usr/lib/ruby/2.5.0/openssl/buffering.rb:316:in `do_write': incompatible character encodings: ASCII-8BIT and UTF-8 (Encoding::CompatibilityError)

At least, this seems to match the original reported error string, so presumably it's a positive.

Next I'll start patch testing.

Bryce Harrington (bryce) wrote :

With just this single patch reverted, the test case passes:

$ sudo patch -R -p2 < /home/ubuntu/0017-Reduce-memory-allocation-when-writing-to-SSLSocket.patch
patching file openssl/buffering.rb

ubuntu@ruby25-sru-1835968-bionic:~$ ./testcase-ruby-ssl.rb
1
2
3
4
5
6
6.16.2Message sent via localhost; From ubuntu@localhost; To: ubuntu@localhost
6.37
You have new mail in /var/mail/ubuntu

Bryce Harrington (bryce) wrote :

I don't know Ruby very well, but I've narrowed it down to a specific line within the patch. The original code had:

    if @sync or @wbuffer.size > BLOCK_SIZE or idx = @wbuffer.rindex("\n")

The patch in question drops the final conditional:

    if @sync or @wbuffer.size > BLOCK_SIZE

The rest of the patch replaces some code that had processed the buffer using the idx with different processing code that doesn't use idx, and I suspect the patch author dropped the condition since idx was no longer needed. However, .rindex("\n") has a side-effect of returning nil when "\n" is not found in the buffer. So in fact, this passes the testcase (but probably breaks lots of other stuff):

    if @sync or @wbuffer.size > BLOCK_SIZE or idx = nil

as does of course

    if @sync or @wbuffer.size > BLOCK_SIZE or nil

So, I think a suitable fix would be to partially restore the line to:

    if @sync or @wbuffer.size > BLOCK_SIZE or @wbuffer.rindex("\n")

This enables the testcase to pass without re-introducing the now-unnecessary idx variable, and leaves the remaining optimization work intact.

Bryce Harrington (bryce) wrote :

As an addendum, poking around some more I downloaded the test file, and noticed it contains a mixture of unix and dos style line encodings:

    $ file example.html
    example.html: HTML document, UTF-8 Unicode text, with very long lines, with CRLF, LF line terminators

Maybe irrelevant, but it's noteworthy that the patch immediately prior to the one flagged above is changing behavior of handling CRLF / LF. Essentially, patch 0016-openssl-buffering.rb-no-RS-when-output.patch is doing:

- (...) idx = @wbuffer.rindex($/)
+ (...) idx = @wbuffer.rindex("\n")

and:

- if $/ && /\n\z/ !~ s
- s << "\n"
- end
+ s.sub!(/(?<!\n)\z/, "\n")

I don't know Ruby enough to know what's going on exactly, but the coincidence seems odd to me, and this code feels worthy of more study.

Bryce Harrington (bryce) wrote :

Unit 193,

I've packaged up a minimal fix into a PPA for testing, can you install it and verify it does indeed work for the various use cases you had tested earlier? It would also be extremely helpful if you could run it against some cases that weren't failing before, just to verify this doesn't cause any secondary regressions.

$ sudo add-apt-repository ppa:bryce/ruby2.5-sru-1835968
$ sudo apt-get update
$ sudo apt-get install -y ruby2.5

Bryce Harrington (bryce) on 2019-07-26
description: updated
Changed in ruby2.5 (Ubuntu Bionic):
status: Triaged → In Progress
Unit 193 (unit193) wrote :

Howdy,

You came up with the same conclusion I had above, including suspecting patch 16 first as well. I've added the repo, upgraded, and we'll see how this goes for a little bit.

~Unit 193

Bryce Harrington (bryce) wrote :

Thanks Unit 193. The changes have successfully passed review, and just await your thumbs up on verification before upload.

Unit 193 (unit193) wrote :

Howdy,

While I haven't done anything specific to put it to the test, I've let cron do its thing and so far haven't hit any issues on either line ending type. So I'm going to say it looks good to me!

~Unit 193

Bryce Harrington (bryce) wrote :

Great to hear, and thanks for the quick feedback. I've finalized and uploaded the package for the SRU team. If they accept, they'll explain next steps.

Steve Langasek (vorlon) wrote :

Bryce,

The existing ruby2.5 2.5.1-1ubuntu1.4 was built in a security-only bileto ppa and copied to the Ubuntu archive so that it would be suitable for publication to bionic-security. (https://launchpad.net/~ci-train-ppa-service/+archive/ubuntu/3662-deletedppa/+build/16681231) Can you please arrange for the same to happen for your latest ruby2.5 upload?

I'll go ahead and reject this upload from the unapproved queue because it will be a needless redundant build that we can't publish to -security. You can still recover the source package from the rejected queue if you need it.

You could also coordinate with the security team to have the ruby2.5 build happen directly in the security team's ppa if they are comfortable with publishing this to -security without further testing.

Bryce Harrington (bryce) wrote :

Hm, I've never used "bileto" before, but thanks was wondering what the rejection was for.

Bryce Harrington (bryce) wrote :

Marc and Andreas are assisting with bileto, will give it a shot.

Hello Unit, or anyone else affected,

Accepted ruby2.5 into bionic-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/ruby2.5/2.5.1-1ubuntu1.5 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-bionic to verification-done-bionic. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-bionic. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

Changed in ruby2.5 (Ubuntu Bionic):
status: In Progress → Fix Committed
tags: added: verification-needed verification-needed-bionic
Unit 193 (unit193) wrote :

Howdy,

Well I tested this before, but the current version in -proposed seems to work just as expected after a few days.

tags: added: verification-done-bionic
removed: verification-needed-bionic
Bryce Harrington (bryce) on 2019-08-07
tags: added: verification-done
removed: verification-needed

The verification of the Stable Release Update for ruby2.5 has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

Launchpad Janitor (janitor) wrote :

This bug was fixed in the package ruby2.5 - 2.5.1-1ubuntu1.5

---------------
ruby2.5 (2.5.1-1ubuntu1.5) bionic; urgency=medium

  * Add d/p/restore_buffer_newline_check.patch to fix failure sending
    files with mixed newline encoding styles; this regression was
    introduced by 0009-openssl-sync-with-upstream-repository.patch.
    (LP: #1835968)

 -- Bryce Harrington <email address hidden> Thu, 25 Jul 2019 16:06:31 -0700

Changed in ruby2.5 (Ubuntu Bionic):
status: Fix Committed → Fix Released
Changed in ruby2.5 (Ubuntu):
status: Confirmed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers