search

mozilla / addons

☂ Umbrella repository for Mozilla Addons ✨
Other
127 stars 41 forks source link

Disallow control characters in filenames inside add-on packages #1831

Open diox opened 4 years ago

diox commented 4 years ago

STR:

Expected results:

Actual results:

Really, having control characters in filenames is asking for trouble, we already do some filename validation in addons-server before it even reaches the linter, we should include a check to prevent control characters (Using unicode definition of a control character - spaces are fine) from being present in any filename.

┆Issue is synchronized with this Jira Task

jvillalobos commented 4 years ago

Should there be an equivalent check in the linter? It sounds like something we can alert developers about before building the XPI so they don't even have to submit it to see the error.

diox commented 4 years ago

I'd still like to have this in addons-server so that it happens early and would also be done any kind of archive file we handle (including source files), but we could duplicate that in the linter, yes.

wagnerand commented 4 years ago

Agreed we should duplicate in the linter so developers notice early, for example during web-ext lint.

willdurand commented 4 years ago

Hi @g-k, it looks like autograph applies some sort of sanitization on filenames. Do you know where we could find how filenames are treated during signing so that we could detect unsupported characters in filenames before hitting autograph?

g-k commented 4 years ago

Hey Will,

filenames need to be:

Autograph trusts client input, so stricter validation in AMO would be good.

willdurand commented 4 years ago

Thanks @g-k!

It looks like control characters should be allowed. I wrote a quick test case in autograph and it passes:

diff --git a/signer/xpi/jar_test.go b/signer/xpi/jar_test.go
index 883ab270..2c88b4ce 100644
--- a/signer/xpi/jar_test.go
+++ b/signer/xpi/jar_test.go
@@ -46,6 +46,22 @@ func TestFormatFilenameInvalidUTF8(t *testing.T) {
    }
 }

+func TestFormatFilenameWithControlCharacter(t *testing.T) {
+   t.Parallel()
+
+   // Both are the same, really.
+   fn := []byte("some/file\x0d")
+   expected := []byte("some/file\r")
+
+   formatted, err := formatFilename(fn)
+   if err != nil {
+       t.Fatal(err)
+   }
+   if !bytes.Equal(formatted, expected) {
+       t.Fatalf("format filename is not the same:\nExpected:\n%q\nGot:\n%q", expected, formatted)
+   }
+}
+
 func TestFormatFilenameTooLong(t *testing.T) {
    t.Parallel()

@g-k I agree AMO should enforce the constraints you listed above and we plan to do this here (AMO) as well as in the linter (a tool used by add-ons developers locally). I am wondering why we would get a signature mismatch currently, though, given that the formatted filename seems to carry the control character. Any idea?

g-k commented 4 years ago

Oh nice! We should land tests like that upstream.

Any idea? I'd guess that either the zip spec or golang archive/zip unpacker treats it differently (like https://bugzilla.mozilla.org/show_bug.cgi?id=1534483) or the control character corrupts the signature manifest file entry.

If you have an example addon I can check that out.

On Tue, Jul 14, 2020 at 12:55 PM William Durand notifications@github.com wrote:

Thanks @g-k https://github.com/g-k!

It looks like control characters should be allowed. I wrote a quick test case in autograph and it passes:

diff --git a/signer/xpi/jar_test.go b/signer/xpi/jar_test.go index 883ab270..2c88b4ce 100644--- a/signer/xpi/jar_test.go+++ b/signer/xpi/jar_test.go@@ -46,6 +46,22 @@ func TestFormatFilenameInvalidUTF8(t testing.T) { } } +func TestFormatFilenameWithControlCharacter(t testing.T) {+ t.Parallel()++ // Both are the same, really.+ fn := []byte("some/file\x0d")+ expected := []byte("some/file\r")++ formatted, err := formatFilename(fn)+ if err != nil {+ t.Fatal(err)+ }+ if !bytes.Equal(formatted, expected) {+ t.Fatalf("format filename is not the same:\nExpected:\n%q\nGot:\n%q", expected, formatted)+ }+}+ func TestFormatFilenameTooLong(t *testing.T) { t.Parallel()

@g-k https://github.com/g-k I agree AMO should enforce the constraints you listed above and we plan to do this here (AMO) as well as in the linter (a tool used by add-ons developers locally). I am wondering why we would get a signature mismatch currently, though, given that the formatted filename seems to carry the control character. Any idea?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mozilla/addons/issues/1831, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABXKLKMV4BZIWO24OJO7HDR3SEZPANCNFSM4ONPV3TA .

willdurand commented 4 years ago

If you have an example addon I can check that out.

@g-k sent via Slack, please keep us posted :)

g-k commented 4 years ago

Thanks Will! Sorry I got pulled away with other stuff for the rest of this week.

I signed the example addon on AMO stage and locally with the options AMO uses and tried stripping the __MACOSX and dot files (which as you mentioned in chat weren't an issue), but in all cases it shows up as corrupt when I tried to install it on my test profile. NSS is usually stricter about validating addons and it'd be good to propagate those checks to AMO and autograph to catch things sooner.

We have some test xpc-shell tests for Fx to give us more information about signing failures in https://github.com/mozilla-services/autograph-canary (let me know if it's a 404 for you), so I was trying to run that against the example addon, but I was running into problems with it not being updated to understand the bloom filter addonblocklist. Do you know if there's a pref to disable that?

I'm also going to try resigning without the COSE signature to see if it's related to that. But I need to remember where Fx checks for the Addon ID field. Does AMO add it to the manifest.json later?

On Fri, Jul 17, 2020 at 6:28 AM William Durand notifications@github.com wrote:

If you have an example addon I can check that out.

@g-k https://github.com/g-k sent via Slack, please keep us posted :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mozilla/addons/issues/1831, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABXKLLQVXYMXU6XATKPJKDR4ARWBANCNFSM4ONPV3TA .

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. If you think this bug should stay open, please comment on the issue with further details. Thank you for your contributions.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. If you think this bug should stay open, please comment on the issue with further details. Thank you for your contributions.

diox commented 3 years ago

Note: doing this on addons-server side ensures it's done before repack and also allows us to apply that check to source uploads (it needs to be done in archive_member_validator() for that).

KevinMind commented 6 months ago

Old Jira Ticket: https://mozilla-hub.atlassian.net/browse/ADDSRV-40