Skip to content

Commit b66a481

Browse files
kareskares
committed
[fix] keep CTR (and OFB) state after final (#392)
JCE finalization resets counter and state after complete blocks; thus later updates diverge - keep active cipher when no output remains to flush
1 parent 0c2ea78 commit b66a481

2 files changed

Lines changed: 95 additions & 9 deletions

File tree

‎src/main/java/org/jruby/ext/openssl/Cipher.java‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1156,6 +1156,7 @@ else if ( "RC4".equalsIgnoreCase(cryptoBase) ) {
11561156

11571157
cipherInited = true;
11581158
processedDataBytes = 0;
1159+
outputDataBytes = 0;
11591160

11601161
if ( switchToManualBlockUpdateBuffer() ) {
11611162
// switch JCE cipher to NoPadding so it flushes complete blocks immediately; add PKCS7 padding in do_final
@@ -1185,6 +1186,7 @@ private String getCipherAlgorithm() {
11851186
}
11861187

11871188
private int processedDataBytes = 0;
1189+
private int outputDataBytes = 0;
11881190
private byte[] lastIV;
11891191

11901192
@JRubyMethod
@@ -1223,6 +1225,7 @@ public IRubyObject update(final ThreadContext context, final IRubyObject arg, IR
12231225
else {
12241226
final byte[] out = cipher.update(in, offset, length);
12251227
if ( out != null ) {
1228+
outputDataBytes += out.length;
12261229
str = new ByteList(out, false);
12271230
if ( realIV != null ) {
12281231
if ( encryptMode ) setLastIVIfNeeded( out );
@@ -1313,8 +1316,14 @@ public IRubyObject do_final(final ThreadContext context) {
13131316
checkAuthTag(runtime);
13141317

13151318
if ( ! cipherInited ) doInitCipher(runtime);
1316-
// trying to allow update after final like cruby-openssl. Bad idea.
1317-
if ( "RC4".equalsIgnoreCase(cryptoBase) ) return runtime.newString("");
1319+
1320+
if ( "RC4".equalsIgnoreCase(cryptoBase) ) return RubyString.newEmptyString(runtime);
1321+
// complete CTR and OFB blocks have nothing to flush (doFinal would reset the state)
1322+
if ( ( "CTR".equalsIgnoreCase(cryptoMode) || "OFB".equalsIgnoreCase(cryptoMode) ) &&
1323+
"NoPadding".equals(paddingType) &&
1324+
processedDataBytes == outputDataBytes && processedDataBytes % ivLength == 0 ) {
1325+
return RubyString.newEmptyString(runtime);
1326+
}
13181327

13191328
final ByteList str;
13201329
try {
@@ -1327,11 +1336,6 @@ else if ( updateBuffer != null ) {
13271336
else {
13281337
final byte[] out = cipher.doFinal();
13291338
if ( out != null ) {
1330-
// TODO: Modifying this line appears to fix the issue, but I do
1331-
// not have a good reason for why. Best I can tell, lastIv needs
1332-
// to be set regardless of encryptMode, so we'll go with this
1333-
// for now. JRUBY-3335.
1334-
//if ( realIV != null && encryptMode ) ...
13351339
str = new ByteList(out, false);
13361340
if ( realIV != null ) setLastIVIfNeeded(out);
13371341
}
@@ -1732,7 +1736,7 @@ private static class AESCipherAllocator implements ObjectAllocator {
17321736
public AES allocate(Ruby runtime, RubyClass klass) {
17331737
return new AES(runtime, klass, keyLength);
17341738
}
1735-
};
1739+
}
17361740

17371741
public static class AES extends Cipher {
17381742
private static final long serialVersionUID = -3627749495034257750L;

‎test/test_cipher.rb‎

Lines changed: 83 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -336,7 +336,7 @@ def test_aes_stream_modes_nist_kat
336336
def test_aes_ctr_streaming_arbitrary_chunks
337337
iv = ['f0f1f2f3f4f5f6f7f8f9fafbfcfdfeff'].pack('H*')
338338
data = ("Streaming CTR keeps a running counter across updates. " * 5).b # 265 bytes
339-
%w[aes-128-ctr aes-256-ctr].each do |algo|
339+
%w[aes-128-ctr aes-192-ctr aes-256-ctr].each do |algo|
340340
key = OpenSSL::Cipher.new(algo).random_key
341341
one = OpenSSL::Cipher.new(algo).encrypt; one.key = key; one.iv = iv
342342
expected = one.update(data) + one.final
@@ -352,6 +352,88 @@ def test_aes_ctr_streaming_arbitrary_chunks
352352
end
353353
end
354354

355+
def test_other_ctr_ciphers_continue_after_final_on_block_boundary
356+
skip_fips_unapproved_ciphers
357+
358+
algos = %w[camellia-128-ctr camellia-192-ctr camellia-256-ctr]
359+
algos += %w[bf-ctr cast5-ctr des-ctr des-ede3-ctr rc2-ctr twofish-ctr] if defined? JRUBY_VERSION
360+
algos.each do |algo|
361+
base = OpenSSL::Cipher.new(algo)
362+
key = "k" * base.key_len
363+
iv = "\0".b * base.iv_len
364+
data = ("0123456789abcdef" * 2).b
365+
366+
whole = OpenSSL::Cipher.new(algo).encrypt; whole.key = key; whole.iv = iv
367+
expected = whole.update(data) + whole.final
368+
369+
split = OpenSSL::Cipher.new(algo).encrypt; split.key = key; split.iv = iv
370+
first = split.update(data.byteslice(0, base.iv_len)) + split.final
371+
second = split.update(data.byteslice(base.iv_len, data.bytesize - base.iv_len)) + split.final
372+
assert_equal expected, first + second, "#{algo} encrypt after final"
373+
end
374+
end
375+
376+
def test_ofb_ciphers_continue_after_final_on_block_boundary
377+
skip_fips_unapproved_ciphers
378+
379+
algos = %w[aes-128-ofb aes-192-ofb aes-256-ofb
380+
camellia-128-ofb camellia-192-ofb camellia-256-ofb]
381+
algos.each do |algo|
382+
base = OpenSSL::Cipher.new(algo)
383+
key = "k" * base.key_len
384+
iv = "v" * base.iv_len
385+
data = ("0123456789abcdef" * 2).b
386+
387+
whole = OpenSSL::Cipher.new(algo).encrypt; whole.key = key; whole.iv = iv
388+
expected = whole.update(data) + whole.final
389+
390+
split = OpenSSL::Cipher.new(algo).encrypt; split.key = key; split.iv = iv
391+
first = split.update(data.byteslice(0, base.iv_len)) + split.final
392+
second = split.update(data.byteslice(base.iv_len, data.bytesize - base.iv_len)) + split.final
393+
assert_equal expected, first + second, "#{algo} encrypt after final"
394+
395+
dec = OpenSSL::Cipher.new(algo).decrypt; dec.key = key; dec.iv = iv
396+
plain = dec.update(first) + dec.final + dec.update(second) + dec.final
397+
assert_equal data, plain, "#{algo} decrypt after final"
398+
end
399+
end
400+
401+
def test_aes_ctr_continues_after_final_on_block_boundary
402+
iv = ['000102030405060708090a0b0c0dfeff'].pack('H*')
403+
data = ("0123456789abcdef" * 4).b
404+
405+
%w[aes-128-ctr aes-192-ctr aes-256-ctr].each do |algo|
406+
key = "k" * OpenSSL::Cipher.new(algo).key_len
407+
whole = OpenSSL::Cipher.new(algo).encrypt; whole.key = key; whole.iv = iv
408+
expected = whole.update(data) + whole.final
409+
410+
enc = OpenSSL::Cipher.new(algo).encrypt; enc.padding = 0; enc.key = key; enc.iv = iv
411+
encrypted = "".b; offset = 0
412+
4.times do
413+
chunk = data.byteslice(offset, 16)
414+
encrypted << enc.update(chunk)
415+
assert_equal "", enc.final, "#{algo} encrypt final"
416+
assert_equal "", enc.final, "#{algo} repeated final"
417+
offset += chunk.bytesize
418+
end
419+
assert_equal expected, encrypted, "#{algo} encrypt after final"
420+
421+
dec = OpenSSL::Cipher.new(algo).decrypt; dec.padding = 0; dec.key = key; dec.iv = iv
422+
plain = "".b; offset = 0
423+
4.times do
424+
chunk = encrypted.byteslice(offset, 16)
425+
plain << dec.update(chunk)
426+
assert_equal "", dec.final, "#{algo} decrypt final"
427+
offset += chunk.bytesize
428+
end
429+
assert_equal data, plain, "#{algo} decrypt after final"
430+
431+
enc.reset
432+
enc.iv = iv
433+
assert_equal expected, enc.update(data) + enc.final, "#{algo} reset with IV after final"
434+
end
435+
end
436+
355437
# NIST SP 800-38A F.5.5/F.5.6 AES-256-CTR known-answer vector
356438
def test_aes256_ctr_nist_kat
357439
key = ['603deb1015ca71be2b73aef0857d77811f352c073b6108d72d9810a30914dff4'].pack('H*')

0 commit comments

Comments
 (0)