Bug #20937: "can't set length of shared string" error when using OpenSSL::Cipher#update with buffer

• Status: Open
• Assignee: rhenium (Kazuki Yamaguchi)
• Target version: -
• ruby -v: ruby 3.3.6 (2024-11-05 revision 75015d4c1f) +YJIT [arm64-darwin23]
• Backport: 3.1: UNKNOWN, 3.2: UNKNOWN, 3.3: UNKNOWN

[akiellor (Andrew Kiellor)]

We've observed a recurring "can't set length of shared string" error in production emerging from the aws-sdk-s3 library when using it's client encryption features. The sdk in this mode uses OpenSSL::Cipher in decrypt mode with a String buffer. It appears that under some circumstances the buffer becomes a "shared string" and is no longer compatible with the requirements of OpenSSL::Cipher#update.

I've attached a reproduction scenario using only the ruby standard library.

$ ruby -v
ruby 3.3.6 (2024-11-05 revision 75015d4c1f) +YJIT [arm64-darwin23]
$ ruby scripty.rb
scripty.rb:32:in `update': can't set length of shared string (RuntimeError)
	from scripty.rb:32:in `block (2 levels) in <main>'
	from scripty.rb:31:in `each'
	from scripty.rb:31:in `block in <main>'
	from /nix/store/fhmbmmzr4h5yax66nl2x44rrdf039b3s-ruby-3.3.6/lib/ruby/3.3.0/tempfile.rb:447:in `create'
	from scripty.rb:26:in `<main>'

The attached script has a few different read patterns, some fail and some do not.

Credit for isolating this issue goes to @simoleone.

[akiellor (Andrew Kiellor)]

• Description updated (diff)

[byroot (Jean Boussier)]

• Related to Bug #20236: OpenSSL::Cipher#update - probable buffer overflow: 16 for 15 added

[byroot (Jean Boussier)]

• Assignee set to rhenium (Kazuki Yamaguchi)

This has been fixed by https://github.com/byroot/openssl/commit/3035559f54eaa42347b9fe2d91bd25a7b0563a44 / https://bugs.ruby-lang.org/issues/20236

But as far as I know it hasn't been released. Deferring to @rhenium (Kazuki Yamaguchi) as to what to do here.

[[e-mail] (Simo Leone)]

I applied the openssl patch as-is from ruby master (https://github.com/ruby/ruby/commit/eb6f0000a4b752803ff7431d24d1a0a535a4387e) to ruby 3.3.6 and confirmed that the patch fixes the issue for all of the example read patterns we identified. Perhaps it's straightforward enough to be a candidate for backport to the next 3.3 release?

[rhenium (Kazuki Yamaguchi)]

This is a different bug. OpenSSL::Cipher#update is failing to make the supplied buffer independent.

Reproducer for master (buffer is 32 bytes larger than input): ruby -ropenssl -e'OpenSSL::Cipher.new("aes-256-ecb").encrypt.tap{_1.random_key}.update("a"*100,("a"*12345)[-132..])'

[byroot (Jean Boussier)]

Ah indeed.

It can be fixed on the openssl side with:

diff --git a/ext/openssl/ossl_cipher.c b/ext/openssl/ossl_cipher.c
index 5a491d8..2e87c60 100644
--- a/ext/openssl/ossl_cipher.c
+++ b/ext/openssl/ossl_cipher.c
@@ -414,6 +414,7 @@ ossl_cipher_update(int argc, VALUE *argv, VALUE self)
     if (!ossl_cipher_update_long(ctx, (unsigned char *)RSTRING_PTR(str), &out_len, in, in_len))
    ossl_raise(eCipherError, NULL);
     assert(out_len <= RSTRING_LEN(str));
+    rb_str_modify(str);
     rb_str_set_len(str, out_len);
 
     return str;

But in a way I find it weird that rb_str_set_len wouldn't make the string independant if it has to, so I think it could be patched on the Ruby side too.

[byroot (Jean Boussier)]

The current rb_str_set_len behavior was added in https://github.com/ruby/ruby/commit/8965ed167dbca9471ccc41e9bebe7e2fb1fa9fcb#diff-39038cbb771e0fad34f253ad6233e5ecce154024017ad118bb401f345aa108c8 by @nobu (Nobuyoshi Nakada), but there is no associated bug report or anything so it's unclear to me whether making the string independent there is acceptable or not.

[byroot (Jean Boussier)]

https://github.com/ruby/openssl/pull/823

[rhenium (Kazuki Yamaguchi)]

The exception from rb_str_set_len() is after the fact, indicating it has illegally written to RSTRING_PTR(str) of a shared string.

Cipher#update uses rb_str_resize() to allocate enough room for the output, but apparently it won't (and never did in previous versions of Ruby) make it independent when the String happens to have the expected length already.

I made an alternative PR: https://github.com/ruby/openssl/pull/824

[rhenium (Kazuki Yamaguchi)]

• Related to deleted (Bug #20236: OpenSSL::Cipher#update - probable buffer overflow: 16 for 15 )