Tcl Library Source Code

View Ticket
Login
Ticket UUID: d56da1abcaa028bed473ff5ffbd814f41909c5f5
Title: Incorrect padding of data when using input channel
Type: Bug Version: 1.0.4
Submitter: anonymous Created on: 2019-06-06 08:43:14
Subsystem: blowfish Assigned To: aku
Priority: 8 Severity: Severe
Status: Closed Last Modified: 2019-11-26 02:55:31
Resolution: Accepted Closed By: aku
    Closed on: 2019-11-26 02:55:31
Description:
Blowfish pads an incoming stream improperly. This was seen and reported by Jim in this thread on comp.lang.tcl:
https://groups.google.com/forum/#!topic/comp.lang.tcl/XhDe5-yLzw0

The improper padding happens in the procedure blowfish::Chunk which already contains a FIXME comment. The procedure needs to care about 2 things:

1. Data must always be processed in multiples of 8 bytes. However, it's not guaranteed that a multiple of 8 bytes is read even in the middle of a stream. Reasons can be an odd user provided chunk size or a nonblocking channel. My patch introduces a new variable state(remainder) to hold the modulo data.

2. Apply padding at the end of the stream, and only there. Don't pad an empty string. Unfortunately, blowfish::Pad is doing exactly this. I think an empty string should not be padded. Likewise an empty data set encrpyts to an empty data set. If this is not correct then extra care has to be taken with the remainig data at EOF. Also, blowfish::Chunk didn't call pad with the user provided pad character.

The attached patch takes care of both points.

Missing: A test for the input channel API.

-- Stephan Effelsberg
User Comments: aku added on 2019-11-26 02:55:31:

Integrated with commit [c230d5a347].

Bumped to version 1.0.5.


aku added on 2019-11-26 00:04:10:

Commit [69558ad9d0].

Done except for full testing. Extended testsuite of module to have cases for -in. Fixed internals, errors got swallowed in the -in code branch.


aku added on 2019-11-25 22:29:34:

Started work, branch [tkt-d56da1abca].

Applied patch. Tweaked Chunk handling slightly. Added missing version changes (code, index, docs).

Commit [ad21f4eab3].

Still to do: Run existing tests, add tests for this ticket.


anonymous added on 2019-06-06 08:45:42:
--- blowfish-1.0.4.tcl	2019-06-06 09:52:21 +0200
+++ blowfish-1.0.5.tcl	2019-06-06 10:02:41 +0200
@@ -509,18 +509,27 @@
     if {[eof $in]} {
         fileevent $in readable {}
         set state(reading) 0
-    }
-
-    set data [read $in $chunksize]
-    # FIX ME: we should ony pad after eof
-    if {[string length $pad] > 0} {
-        set data [Pad $data 8]
-    }
-
-    if {$out == {}} {
-        append state(output) [$state(cmd) $Key $data]
+        set data $state(remainder)
+        # Only pad at the end of the stream.
+        if {[string length $pad] > 0} {
+            set data [Pad $data 8 $pad]
+        }
     } else {
-        puts -nonewline $out [$state(cmd) $Key $data]
+        set data [read $in $chunksize]
+        puts "Chunk: reading [string len $data] bytes"
+        set data $state(remainder)$data
+        # If data is not a multiple of 8, state(remainder) will hold
+        # excess bytes for the next round.
+        set pagedlen [expr {([string length $data] / 8) * 8}]
+        set state(remainder) [string range $data $pagedlen end]
+        set data [string range $data 0 $pagedlen-1]
+    }
+    if {[string length $data] > 0} {
+        if {$out == {}} {
+            append state(output) [$state(cmd) $Key $data]
+        } else {
+            puts -nonewline $out [$state(cmd) $Key $data]
+        }
     }
 }
 
@@ -585,7 +594,7 @@
 proc ::blowfish::Pad {data blocksize {fill \0}} {
     set len [string length $data]
     if {$len == 0} {
-        set data [string repeat $fill $blocksize]
+        # do not pad an empty string
     } elseif {($len % $blocksize) != 0} {
         set pad [expr {$blocksize - ($len % $blocksize)}]
         append data [string repeat $fill $pad]
@@ -682,6 +691,7 @@
             set state(cmd) Decrypt
         }
         set state(output) ""
+        set state(remainder) ""
         fileevent $opts(-in) readable \
             [list [namespace origin Chunk] \
                  $Key $opts(-in) $opts(-out) $opts(-chunksize) $opts(-pad)]