From 16ec13b991d9dd213bfb239be109a42b2d2c16bb Mon Sep 17 00:00:00 2001 From: jangko Date: Fri, 11 Jun 2021 09:10:07 +0700 Subject: [PATCH] fixes gzip security bug never assume data coming from network always valid --- .github/workflows/ci.yml | 9 ++-- miniz.nimble | 1 + miniz/gzip.nim | 94 ++++++++++++++++++++++++---------------- tests/test_gzip.nim | 13 +++--- 4 files changed, 70 insertions(+), 47 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3484429..7bde3f3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -10,15 +10,15 @@ jobs: test_lang: [c, cpp] target: - os: linux - cpu: amd64 + cpu: amd64 - os: linux - cpu: i386 + cpu: i386 - os: macos cpu: amd64 - os: windows - cpu: amd64 + cpu: amd64 - os: windows - cpu: i386 + cpu: i386 include: - target: os: linux @@ -149,4 +149,5 @@ jobs: shell: bash working-directory: nim-miniz run: | + nimble install -y --depsOnly env TEST_LANG="${{ matrix.test_lang }}" nimble test diff --git a/miniz.nimble b/miniz.nimble index f2fe16e..468126f 100644 --- a/miniz.nimble +++ b/miniz.nimble @@ -17,6 +17,7 @@ license = "Apache License 2.0" skipDirs = @["tests"] requires "nim >= 1.2.0" +requires "stew >= 0.1.0" # Helper functions proc test(env, path: string) = diff --git a/miniz/gzip.nim b/miniz/gzip.nim index ad07142..da7adfd 100644 --- a/miniz/gzip.nim +++ b/miniz/gzip.nim @@ -8,9 +8,10 @@ # those terms. import + stew/results, ./miniz_api -proc gzip*[T: byte|char](N: type, source: openArray[T]): N = +proc gzip*[T: byte|char](N: type, source: openArray[T]): Result[N, string] = # all these cast[ptr cuchar] is need because # clang++ will complaints about incompatible # pointer types @@ -22,57 +23,66 @@ proc gzip*[T: byte|char](N: type, source: openArray[T]): N = avail_in: source.len.cuint ) - assert(mz.deflateInit2( + var r = mz.deflateInit2( MZ_DEFAULT_LEVEL, MZ_DEFLATED, MZ_RAW_DEFLATE, MZ_DEFAULT_MEM_LEVEL, - MZ_DEFAULT_STRATEGY) == MZ_OK - ) + MZ_DEFAULT_STRATEGY) + + if r != MZ_OK: + return err($r) let maxSize = mz.deflateBound(source.len.culong).int when N is string: type CC = char - result = newString(maxSize + 18) + var res = newString(maxSize + 18) elif N is seq[byte]: type CC = byte - result = newSeq[byte](maxSize + 18) + var res = newSeq[byte](maxSize + 18) else: {.fatal: "unsupported output type".} - result[0] = 0x1F.CC - result[1] = 0x8B.CC - result[2] = 8.CC - result[3] = 0.CC - result[4] = 0.CC - result[5] = 0.CC - result[6] = 0.CC - result[7] = 0.CC - result[8] = 0.CC - result[9] = 0xFF.CC + res[0] = 0x1F.CC + res[1] = 0x8B.CC + res[2] = 8.CC + res[3] = 0.CC + res[4] = 0.CC + res[5] = 0.CC + res[6] = 0.CC + res[7] = 0.CC + res[8] = 0.CC + res[9] = 0xFF.CC - mz.next_out = cast[ptr cuchar](result[10].addr) - mz.avail_out = (result.len - 10).cuint - assert(mz.deflate(MZ_FINISH) == MZ_STREAM_END) + mz.next_out = cast[ptr cuchar](res[10].addr) + mz.avail_out = (res.len - 10).cuint + r = mz.deflate(MZ_FINISH) + + if r != MZ_STREAM_END: + return err($r) let size = mz.total_out.int crc = mz_crc32(source) ssize = source.len - result[size + 10] = CC( crc and 0xFF) - result[size + 11] = CC((crc shr 8) and 0xFF) - result[size + 12] = CC((crc shr 16) and 0xFF) - result[size + 13] = CC((crc shr 24) and 0xFF) - result[size + 14] = CC( ssize and 0xFF) - result[size + 15] = CC((ssize shr 8) and 0xFF) - result[size + 16] = CC((ssize shr 16) and 0xFF) - result[size + 17] = CC((ssize shr 24) and 0xFF) + res[size + 10] = CC( crc and 0xFF) + res[size + 11] = CC((crc shr 8) and 0xFF) + res[size + 12] = CC((crc shr 16) and 0xFF) + res[size + 13] = CC((crc shr 24) and 0xFF) + res[size + 14] = CC( ssize and 0xFF) + res[size + 15] = CC((ssize shr 8) and 0xFF) + res[size + 16] = CC((ssize shr 16) and 0xFF) + res[size + 17] = CC((ssize shr 24) and 0xFF) - result.setLen(mz.total_out.int + 18) - assert(mz.deflateEnd() == MZ_OK) + res.setLen(mz.total_out.int + 18) + r = mz.deflateEnd() + if r != MZ_OK: + return err($r) -proc ungzip*[T: byte|char](N: type, data: openArray[T]): N = + ok(res) + +proc ungzip*[T: byte|char](N: type, data: openArray[T]): Result[N, string] = var mz = MzStream( next_in: if data.len == 0: nil @@ -82,28 +92,38 @@ proc ungzip*[T: byte|char](N: type, data: openArray[T]): N = ) const windowBits = MZ_RAW_DEFLATE - doAssert(mz.inflateInit2(windowBits) == MZ_OK) + var r = mz.inflateInit2(windowBits) + + if r != MZ_OK: + return err($r) + var res: seq[byte] var buf: array[0xFFFF, byte] while true: mz.next_out = cast[ptr cuchar](buf[0].addr) mz.avail_out = buf.len.cuint - let r = mz.inflate(MZ_SYNC_FLUSH) + r = mz.inflate(MZ_SYNC_FLUSH) let outSize = buf.len - mz.avail_out.int res.add toOpenArray(buf, 0, outSize-1) if r == MZ_STREAM_END: break elif r == MZ_OK: - continue + # need more input or more output available + if mz.avail_in > 0 or mz.avail_out == 0: + continue + else: + break else: - doAssert(false, "decompression error") + return err("decompression error: " & $r) - doAssert(mz.inflateEnd() == MZ_OK) + r = mz.inflateEnd() + if r != MZ_OK: + return err($r) when N is string: - cast[string](res) + ok(cast[string](res)) elif N is seq[byte]: - res + ok(res) else: {.fatal: "unsupported output type".} diff --git a/tests/test_gzip.nim b/tests/test_gzip.nim index 29953f3..038b595 100644 --- a/tests/test_gzip.nim +++ b/tests/test_gzip.nim @@ -9,6 +9,7 @@ import std/[unittest, os], + stew/results, ../miniz/gzip proc toBytes(s: string): seq[byte] = @@ -24,14 +25,14 @@ suite "gzip test suite": let parts = splitFile(path) test parts.name: let s = readFile(path) - let cstr = string.gzip(s) - let cbytes = seq[byte].gzip(s) + let cstr = string.gzip(s).get() + let cbytes = seq[byte].gzip(s).get() check cbytes.len == cstr.len - let dcstr = string.ungzip(cbytes) - let dcstr2 = string.ungzip(cstr) - let dcbytes = seq[byte].ungzip(cbytes) - let dcbytes2 = seq[byte].ungzip(cstr) + let dcstr = string.ungzip(cbytes).get() + let dcstr2 = string.ungzip(cstr).get() + let dcbytes = seq[byte].ungzip(cbytes).get() + let dcbytes2 = seq[byte].ungzip(cstr).get() check dcbytes2 == s.toBytes check dcbytes == s.toBytes check dcstr2 == s