Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

archive/zip: ignore malformed Extra field #13166

Closed
sintanial opened this issue Nov 5, 2015 · 10 comments
Closed

archive/zip: ignore malformed Extra field #13166

sintanial opened this issue Nov 5, 2015 · 10 comments
Labels
FrozenDueToAge
Milestone
Go1.6Maybe

Comments

@sintanial
Copy link

I have problem with golang zip reader. We have apk builder which build apk and then encrypted this apk with dex protector.

When i try to open apk file, go return err "zip: not a valid zip file". But when i try to open with unzip or zipinfo, worked fine.
(Also I noticed something interesting: in php 5.5 apk file opened with ZipArchive fine, but in 5.6 return error like golang)

I try open to debug golang archive/zip pckage and found that error returned in this line https://github.com/golang/go/blob/master/src/archive/zip/reader.go#L269.

Then i try to debug this line like this:

fmt.Printf("%+v", f)
fmt.Println(
    "len(f.Extra):", len(f.Extra),
    "len(b):", len(b),
    "tag:", tag,
    "size:", size,
)

And have this results:

&{FileHeader:{
Name:META-INF/MANIFEST.MF CreatorVersion:20 ReaderVersion:20 Flags:2056 Method:8 
ModifiedTime:23093 ModifiedDate:18230 CRC32:2569174240 
CompressedSize:359 UncompressedSize:570 CompressedSize64:359 UncompressedSize64:570 
Extra:[] ExternalAttrs:0 Comment:
} zipr:0xc208038038 zipsize:276893 headerOffset:0}

&{FileHeader:{
Name:res/drawable/ic_launcher.png CreatorVersion:10 ReaderVersion:10 Flags:2048 Method:0 
ModifiedTime:22030 ModifiedDate:18230 CRC32:2310989215 
CompressedSize:20418 UncompressedSize:20418 CompressedSize64:20418 UncompressedSize64:20418
Extra:[] ExternalAttrs:0 Comment:
} zipr:0xc208038038 zipsize:276893 headerOffset:2066}

...

---------------->PROBLEM HERE:-------------------=>
&{FileHeader:{
Name:AndroidManifest.xml CreatorVersion:10 ReaderVersion:10 Flags:2048 Method:0 
ModifiedTime:22030 ModifiedDate:18230 CRC32:1929033187 
CompressedSize:7268 UncompressedSize:7268 CompressedSize64:7268 UncompressedSize64:7268 
Extra:[204 94 136 27 148 14 49 95 154 4 231 53 127 242 4 199 243 207 229 161 103 90 116 69 45 31 54 220 113 43 172 4 159 109 211 57 217 2 161 17 196 16 168 19 174 138 107 15 36 181 223 22 15 21 106 153] 
ExternalAttrs:0 Comment:
} zipr:0xc208038038 zipsize:276893 headerOffset:22954}

len(f.Extra): 56
len(b): 52
tag: 24268 
size: 7048

Problem in extra headers (i think this extra headers add dex protector when encrypt apk). Size more then length of extra bytes. But unzip util and ZipArchive in php5.5 woked perfect with this extras. How to fix this ?

I want to rewrite this piece in go archive/zip package, so that the extras do not interfere golang zip reader.

It's very important for me, because i rewrite build apk from php to golang and I came across a problem that can not decide :(. Please help me.
@sintanial sintanial changed the title Error when try to open apk with extra field in some file archive/zip: Error when try to open apk with extra field in some file Nov 5, 2015
@bradfitz
Copy link
Contributor

bradfitz commented Nov 5, 2015

Can you ask "dex protector" to fix their zip code, if they're producing non-compliant zip files? (the fact that both Go and later PHP releases reject it is interesting, suggesting that the problem isn't with Go)

@sintanial
Copy link
Author

Mmm
Unfortunately, I think my review "dex protector" ignored.
But unix zip util worked fine with this archive, do you now why ? He ignored this extra fields ?

Brad can you help me :).
Can you tell me how to fast fix this in go code :) ?
Because my project worked with 1.5 terabyte apk files, and i can replace all files, with new archive :)

@bradfitz
Copy link
Contributor

bradfitz commented Nov 5, 2015

No, sorry, I can't help you quickly. I don't have time right now to investigate and fully understand this bug report on short notice, especially because it sounds like the answer is that the zip file is just invalid.

@sintanial
Copy link
Author

:( ok, thx

@bradfitz
Copy link
Contributor

bradfitz commented Nov 5, 2015

Please provide a reference to your bug report for dex protector.

@sintanial
Copy link
Author

I was thinking.
Why in extras field, when size > remaining number of bytes, then go return error.
It's only extra info, some additional information, which does not affect on files. Ok, maybe who created this archive, wrong with size in extras ( or maybe not, and it's some special cases), but it's doesn't corrupted archive and file.
Maybe it's not an important check and it can be removed ??

@bradfitz
Copy link
Contributor

bradfitz commented Nov 6, 2015

Where is your dex protector bug report?

I don't want to consider fixing this here until the creator of the problem has already considered fixing it there.

Also, I would need an example of such a corrupt zip.

@rsc
Copy link
Contributor

rsc commented Dec 28, 2015

The extra field contains these bytes:

00000000  cc 5e 88 1b 94 0e 31 5f  9a 04 e7 35 7f f2 04 c7  |.^....1_...5....|
00000010  f3 cf e5 a1 67 5a 74 45  2d 1f 36 dc 71 2b ac 04  |....gZtE-.6.q+..|
00000020  9f 6d d3 39 d9 02 a1 11  c4 10 a8 13 ae 8a 6b 0f  |.m.9..........k.|
00000030  24 b5 df 16 0f 15 6a 99                           |$.....j.|

That's obviously wrong, since it's supposed to contain a sequence of entries each beginning with a 2-byte tag + 2-byte size. Maybe dexprotector even does this intentionally, to make some analysis tools reject the zip file.

However, this is something that was added late to zip files, and early versions of the tools are expected to ignore the extra fields, and most do. The only reason we parse the extra fields is to look for the zip64 extension. Obviously it's not present in this file, so I think rather than return ErrFormat we should just stop looking and keep going.

This is the second time we've had problems with our extra field parsing being too strict. Let's just make it best effort and be done with it. It's clearly something zip implementations disagree about.

@rsc rsc added this to the Go1.6Maybe milestone Dec 28, 2015
@rsc rsc changed the title archive/zip: Error when try to open apk with extra field in some file archive/zip: ignore malformed Extra field Dec 28, 2015
@rsc rsc mentioned this issue Dec 28, 2015
Closed
@sintanial
Copy link
Author

+1
@rsc I totally agree with you.

@gopherbot
Copy link

CL https://golang.org/cl/18317 mentions this issue.

@rsc rsc closed this as completed in 4aedbf5 Jan 7, 2016
@golang golang locked and limited conversation to collaborators Jan 7, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.6Maybe
Development

No branches or pull requests
4 participants
@bradfitz @rsc @sintanial @gopherbot