archive/tar: missed writing 500 bytes - JDK11

[woohgit]

We're upgrading our installer from JDK8 to JDK11 and the generated MSI files can't be signed.

The error is:

archive/tar: missed writing 500 bytes

We generate the MSI files with the wix toolset (https://wixtoolset.org/documentation/manual/v3/overview/alltools.html)

The way how we generate the MSIs is:

unzip jre.zip -d tmp
JREDIR=tmp/jdk-11.0.16.1+1-jre
echo "JRE=$JREDIR"
heat dir "$JREDIR" -o jre.wxs -sfrag -sreg -nologo -srd -gg -cg JreComponents -dr JreDir -var var.JreDir

# pick up java.exe File ID
JavaExeId=$(grep java.exe jre.wxs | grep -o "fil[0-9A-F]*")

candle -dJreDir="$JREDIR" -dWAR="$war" -dJavaExeId=$JavaExeId -nologo -ext WixUIExtension -ext WixUtilExtension -ext WixFirewallExtension jenkins.wxs jre.wxs
# '-sval' skips validation. without this, light somehow doesn't work on automated build environment
# set to -dcl:low during debug and -dcl:high for release
light -o @@ARTIFACTNAME@@.msi -sval -nologo -dcl:high -cultures:en-us -loc en_us.wxl -ext WixUIExtension -ext WixUtilExtension -ext WixFirewallExtension jenkins.wixobj jre.wixobj

If you're interested I can share the MSI installer with you for debug purposes.

If we use the JDK8 JRE, the signing process works fine, it only fails with JDK11.

[bwalding]

We investigated this a bit further - the msitar.go msiToTarDir routine is reading a length of 8692 bytes in the item header for the comdoc.DirStream. But then the stream from the MSI is ACTUALLY only 8192 bytes.

The 'faulty object' is the Table._StringPool.

            SummaryInformation: Declared=     524  Actual=     524
          Binary.WixFirewallCA: Declared=   80896  Actual=   80896
...
             Table._StringData: Declared=   73625  Actual=   73625
             Table._StringPool: Declared=    8692  Actual=    8192
                   Table.Error: Declared=       4  Actual=       4

Now what we don't know is...

1. is the MSI corrupt?
2. is the comdoc.ComDoc ListDir reader broken - and reading the wrong stream size?
3. is something else wrong?

[milescrabill]

I have run into this as well.

The issue surfaces in this code: https://github.com/sassoftware/relic/blob/master/lib/authenticode/msitar.go#L107-L121.

The problem is that in some cases, item.StreamSize does not accurately reflect the size of the stream / reader returned by cdf.ReadStream(item). I'm not sure exactly why, and my wading into the comdoc code has not yielded results.

For certain .msi files this error is reproducible every time signing is attempted.

If you compare the number of bytes actually read by the io.Copy() to the item.StreamSize, the difference between two is the number of bytes that archive/tar reports missing.

Here's a patch with my hacky non-production fix:

diff --git a/lib/authenticode/msitar.go b/lib/authenticode/msitar.go
index b5be078..a79c655 100644
--- a/lib/authenticode/msitar.go
+++ b/lib/authenticode/msitar.go
@@ -20,6 +20,7 @@ import (
    "archive/tar"
    "bytes"
    "crypto"
+   "fmt"
    "io"
    "io/ioutil"

@@ -108,16 +109,20 @@ func msiToTarDir(cdf *comdoc.ComDoc, tw *tar.Writer, parent *comdoc.DirEnt, path
            if err != nil {
                return err
            }
+           data, err := io.ReadAll(r)
+           if err != nil {
+               return err
+           }
            hdr := &tar.Header{
                Name: itemPath,
                Mode: 0644,
-               Size: int64(item.StreamSize),
+               Size: int64(len(data)),
            }
            if err := tw.WriteHeader(hdr); err != nil {
                return err
            }
-           if _, err := io.Copy(tw, r); err != nil {
-               return err
+           if _, err := io.Copy(tw, bytes.NewReader(data)); err != nil {
+               return fmt.Errorf("msiToTarDir(): %w", err)
            }
        case comdoc.DirStorage:
            if err := msiToTarDir(cdf, tw, item, itemPath+"/"); err != nil {

With that code change, which prefers the actual size of the stream to the item.StreamSize's reporting, I can sign otherwise broken .msis just fine.

[mtharp]

Thanks so much for digging into this. Is it possible to provide a sanitized MSI that exhibits the issue?

I get the gist of what's going on, and the change in the PR is probably headed in the right direction, but it looks like in certain circumstances it might end up reading garbage. Ideally readSector should return ErrUnexpectedEOF on partial read like how io.ReadAtLeast works. But that by itself isn't enough as the caller then has to do something reasonable about the missing bytes, not just recycle whatever was left in the buffer from last time.

It might be better for the client to zero-pad the file to the expected size first so that the result is unambiguous instead of relying on the validator having the same quirky interpretation as relic did when it signed it.

[woohgit]

@mtharp Hey, I uploaded an MSI that causes issues for us. https://papai.jp/cloudbees-core-oc-2.375.2.2.zip

It's unsigned for now. The zip contains the MSI.

[mtharp]

I was able to confirm that all the streams in the MSI are intact. The authoring tool just neglected to pad out the last sector, and the low-level reads in relic let the EOF error propagate in a way it shouldn't have which is why the error message is so mystifying. Now it should correctly handle the short read, and raise a correct error if the file really is truncated. With the 7ac277a fix I was able to sign your MSI and validate it from Windows.

Thanks to everyone for digging in, if you can give the fix a try and let me know if it works for you then I'll close this out and cut a release.

[woohgit]

@mtharp I tried the new code and it looks like I can't sign things that I could sign before (an exe file for examle)

09:55:29  Error: can't sign file
09:55:29    Cause: PE sections are out of order

And can't even sign the MSI that I sent to you. Not sure what's going on but it looks like that the fix just made it worse than it was before.

Siging the MSI I sent to you yesterday:

Error: can't sign file
  Cause: archive/tar: missed writing 424 bytes

UPDATE ignore me, I might have messed up the import, I'm using v7.2.1+incompatible which is pretty old... let me try it again.

[mtharp]

Yes, 7.2.1 was the last tagged release before go.mod was added. If you're importing relic from your own code make sure you're importing it with the major version i.e. github.com/sassoftware/relic/v7 and then go get -d github.com/sassoftware/relic/v7@master to move to the latest commit.

If you just want a prebuilt binary you can grab the output from actions (scroll to the bottom): https://github.com/sassoftware/relic/actions/runs/3952558676

[woohgit]

@mtharp It seems to be working! It's awesome news

[chrisroberts]

Also tested and confirm it's working as well :slightly_smiling_face:

[mtharp]

Excellent, thanks everyone. v7.5.4 is released with the fix.