Bugzilla

Sorry, I've just seen this and it's now past midnight here. I'm at a wedding and am not going to be able to write a patch right now.

(In reply to Dave Townsend [:mossop] (he/him) from comment #21)
> Ah. We should probably call it from nsBrowserApp I guess.

This is a good idea, but the problem is that we also removed the osint checks elsewhere, and so just not checking anything in TB would potentially reopen security issues for thunderbird w/ mailto: (or Thunderbird<installhash>:) protocol handling.

I expect that we should call it from the integration points of various apps (ie from nsBrowserApp rather than XRE code) but also make the helper function take arguments that list valid params + whether or not they take an argument. That or write a similar checking function in mail code. This would require some work from someone who knows mail code to do it at the right place in mail code...

A slightly more short-term/impromptu (for 68.1 ?) solution might be ifdefs in EnsureCommandlineSafe based on MOZ_BUILD_APP that accept -mail / -compose (with/without params), though that feels pretty yucky.

I can try to get a patch written on my (UK time) Monday, but probably not before then. Keeping needinfo for that. Even then, I likely can't test the mail side of the patch so it'd be helpful if someone who knows mail code could write and/or test the patch.

(In reply to Jorg K (GMT+2) from comment #24)
> Thanks to all who have commented. A few remarks:
> 
> 1. The crash was reported in bug 1577796. Should I dupe this here? Looks like a "no" reading the end of comment #22.
I think "no" is right - we added exit() calls, but those shouldn't crash, per se...

> 2. Can some of the people involved in bug 1572838 fix this quickly (and move it out of MailNews).

See above. The patch shouldn't be too difficult to write so if you have time before me.

> 3. I can't ship TB 68.1 with this issue. I assume you're planning to backport to FF 60.9?

60esr isn't vulnerable to the issue in the bug as written, so no.

> In this case TB 60.9 would inherit the problem as well. I can fix it with a backout of bug 1572838 from our release branches, but backing out a security fix is likely a bad idea. But if calling it from nsBrowserApp.cpp which is your "main" is an option, TB wouldn't be affected and a backout would be a valid short term fix. Please advise.

See above - just backing this out will likely net you the same security issue as Firefox had prior to the patch. Moving to nsBrowserApp without an equivalent check in mail code would do the same.

> 4. Can you give me access to bug 1572838 so I can see what this is about.

Someone else seems to have done this.

Sorry, I've just seen this and it's now past midnight here. I'm at a wedding and am not going to be able to write a patch right now.

(In reply to Dave Townsend [:mossop] (he/him) from comment #21)
> Ah. We should probably call it from nsBrowserApp I guess.

This is a good idea, but the problem is that we also removed the osint checks elsewhere, and so just not checking anything in TB would potentially reopen security issues for thunderbird w/ mailto: (or Thunderbird<installhash>:) protocol handling.

I expect that we should call it from the integration points of various apps (ie from nsBrowserApp rather than XRE code) but also make the helper function take arguments that list valid params + whether or not they take an argument. That or write a similar checking function in mail code. This would require some work from someone who knows mail code to do it at the right place in mail code...

A slightly more short-term/impromptu (for 68.1 ?) solution might be ifdefs in EnsureCommandlineSafe based on MOZ_BUILD_APP that accept -mail / -compose (with/without params), though that feels pretty yucky.

I can try to get a patch written on my (UK time) Monday, but probably not before then. Keeping needinfo for that. Even then, I likely can't test the mail side of the patch so it'd be helpful if someone who knows mail code could write and/or test the patch.

(In reply to Jorg K (GMT+2) from comment #24)
> Thanks to all who have commented. A few remarks:
> 
> 1. The crash was reported in bug 1577796. Should I dupe this here? Looks like a "no" reading the end of comment #22.

I think "no" is right - we added exit() calls, but those shouldn't crash, per se...

> 2. Can some of the people involved in bug 1572838 fix this quickly (and move it out of MailNews).

See above. The patch shouldn't be too difficult to write so if you have time before me.

> 3. I can't ship TB 68.1 with this issue. I assume you're planning to backport to FF 60.9?

60esr isn't vulnerable to the issue in the bug as written, so no.

> In this case TB 60.9 would inherit the problem as well. I can fix it with a backout of bug 1572838 from our release branches, but backing out a security fix is likely a bad idea. But if calling it from nsBrowserApp.cpp which is your "main" is an option, TB wouldn't be affected and a backout would be a valid short term fix. Please advise.

See above - just backing this out will likely net you the same security issue as Firefox had prior to the patch. Moving to nsBrowserApp without an equivalent check in mail code would do the same.

> 4. Can you give me access to bug 1572838 so I can see what this is about.

Someone else seems to have done this.