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

NSS 3.35 busts Content Permissions in Basilisk #265

Closed
wolfbeast opened this issue Apr 24, 2018 · 41 comments

Comments

@wolfbeast
Copy link
Member

commented Apr 24, 2018

As reported on the forum:

In Basilisk-20180424, all of my cookie permission exceptions are gone.
(They display nowhere, have no effect - and new ones, though effective for the session, cannot be permanently saved.)

The "Exceptions" window (in custom settings for history) is empty.
They're gone from the "Permissions" pane of "Page Info".

New cookie permissions could be created for the current session, but are gone after a restart.

After rolling back to Basilisk-20180321, the cookie exceptions came back like normal.
(For clarity, brand new exceptions created in v20180424 did not come back, but all earlier ones did.)

Tried a new profile: same result.

@JustOff

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2018

Browser Date Result
Basilisk Moebius 2018.03.21 Ok
Basilisk UXP 2018.04.23 Fail
Basilisk UXP Pre-PM28 2018.03.29 Fail
Basilisk UXP 2018.03.19 (cca 12:00) Fail
Basilisk UXP Pre-path-traversal-restriction 2018.03.18 Fail
Basilisk UXP Pre-e10-UI-cleanup 2018.03.03 Fail
Basilisk UXP Pre-PR#34 2018.03.01 Fail
Basilisk UXP Post-NSS-3.35-merge 2018.02.24 Fail
TLS 1.3 branch NSS update 2018.02.23 Fail
TLS 1.3 branch NSPR update only 2018.02.23 Ok
Basilisk UXP Pre-TLS1.3-merge 2018.02.23 Ok
Basilisk UXP Pre-NSS-3.35 2018.02.23 Ok
Basilisk UXP CP1 2018.02.11 Ok

PS : Pale Moon 28.0.0a1 does not allow to add exception for cookies at all - throws error in the console.

@janekptacijarabaci

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2018

Pale Moon 28.0.0a1 does not allow to add exception for cookies at all - throws error in the console.

Bug 1173523 Change nsIPermission to expose a principal rather than a host, appId, and inBrowserElement

This also relates to 5) in:
#121 (comment)
(I'm not doing it now)

@janekptacijarabaci

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2018

@JustOff

FYI (I couldn't try it earlier):

Browser Date Result
Basilisk UXP 2018.03.19 (cca 12:00) Fail

(I don't have an exact commit)

@mattatobin

This comment has been minimized.

Copy link
Member

commented Apr 25, 2018

This is wider than JUST cookies.. No site permissions are being preserved.

@wolfbeast

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2018

Building 912aa47 pre path-traversal-restriction (2018.03.18)

Result: Fail.

@janekptacijarabaci

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2018

I can build e.g. d2a0a16 pre ported-upstream (2018.03.15)

I don't know when... sometimes today.

@JustOff

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2018

Pre-NSS-3.35 -> Ok

Building from Pre-e10-UI-cleanup 2018.03.03 ...

@wolfbeast

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2018

We'll get there.
I'll build the commit right before the batch-import PR #34 (2018.03.01)

Result: Fail

@wolfbeast

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2018

I really hope this isn't NSS fallout

@mattatobin

This comment has been minimized.

Copy link
Member

commented Apr 25, 2018

I don't see how.. Pale Moon and my Navigator have zero issues with Site Permissions.

@wolfbeast

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2018

NSS 3.35 made Tycho unstable as hell.. while there's no clear reason for that either.
so....

@JustOff

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2018

My PC is so slow, so it takes about 90 minutes for each full build. Nonetheless I'm going to continue, please keep our research in sync.

I'm going to build from Post-NSS-3.35-merge 2018.02.24.

@wolfbeast

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2018

Yeah I clock about half that per build. I'll try the last commit prior to the tls 1.3 branch merge. 6f93b00

@wolfbeast

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2018

Last commit prior to TLS 1.3 merge (NSS 3.35) = Ok. So it's likely more NSS 3.35 fallout.

@JustOff

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2018

Well, we nailed it: Post-NSS-3.35-merge - Fail 😠

Tag #28.

@wolfbeast

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2018

As far as I can gather it's because of the partial rewrite Mozilla did with NSS 3.35, and I think NSS's internal sqlite driver is now interfering with the normal sqlite lib. It would also explain the Pale Moon crashes and instability we've seen on Tycho with it. I'll toss this ball upstream, but it's possible Mozilla simply doesn't give a shit.

@wolfbeast

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2018

Verified with the last in-branch build without any other commits: NSS update to 3.35 breaks it. NSPR is fine.

@JustOff

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2018

Just wondering if there is a chance NSS 3.36.1 can help?

@mattatobin

This comment has been minimized.

Copy link
Member

commented Apr 25, 2018

NSS 3.36 requires NSPR 4.19 or newer

@wolfbeast

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2018

Not until https://bugzilla.mozilla.org/show_bug.cgi?id=1443020 at least has some clarity on what is going on. Just trial-and-erroring here is NOT a good thing to do with a lib that is at the heart of all secure connections.

@JustOff

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2018

I'm afraid Mozilla is just as interested in discussing compatibility between NSS 3.35 and ESR 52, as Pale Moon core developers - in supporting Windows XP :-/

@wolfbeast

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2018

There is a big difference here. NSS is NOT a Firefox-exclusive library, and if we run into these issues here, other consumers of NSS can (and likely will) run into similar problems sooner or later. We're also not asking for support of a long since EoL OS -- ESR52 is, in fact, still supported and updated by Mozilla, to boot. You can't compare the two.

@mattatobin mattatobin changed the title Cookie exceptions not read or stored NSS 3.35 busts Content Permissions in Basilisk Apr 25, 2018

@mattatobin mattatobin added the Critical label Apr 25, 2018

@JustOff

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2018

You are probably right, and my view of this case is rather limited, so I'm not trying to judge anyone, I just think out loud.

mattatobin added a commit that referenced this issue Apr 26, 2018
@JustOff

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2018

It seems I finally was able to get NSS 3.35 to work properly with UXP, there are even two ways to achieve this.

The first method is to force the storage service to initialize first in nsNSSComponent.cpp (as done in the FreeBSD firefox-esr port), and the second one is to change the default storage file format back to DBM (as recommended by one of the NSS contributors).

I made test builds of Basilisk for both options ([1], [2]) and both save and restore Content Permissions without any problems.

I think it's better to use the second option (keep using DBM), as it looks more safe and keeps the backward compatibility of the db files.

@wolfbeast

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2018

So, means I was right in that it's a clash with the SQLite driver.
Thanks for digging -- reverting to DBM is the right call; especially since Mozilla isn't telling anyone in that bug which changes were made to Firefox to make it fully NSS-SQL compatible.

@wolfbeast

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2018

So in effect this means we'll have to get NSS, patch it, and enforce in-tree use of NSS again like we did before for Camellia-GCM.

@wolfbeast wolfbeast self-assigned this Jun 5, 2018

@mattatobin

This comment has been minimized.

Copy link
Member

commented Jun 5, 2018

This time I recommend completely removing the ability to build using system nss so it can't be subverted by patchers. If mozconfigure complains about the option being invalid so much the better. (Haven't looked up where it lives).

@wolfbeast

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2018

Considering this will be a "from this point forward" thing, I fully agree.

@JustOff

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2018

It seems that there is simply no other option.

Unless we find a way to explicitly request to use DBM, by adding a type prefix to the database directory, but I can't figure out how to do this.

@mattatobin

This comment has been minimized.

Copy link
Member

commented Jun 5, 2018

And you won't because Mozilla will likely remove support code for it in the future. NSS is forked, again, best track upstream and apply/port patches from here on in.

Goanna Security Services anyone? ;)

@JustOff

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2018

While I was digging into NSS, I found an interesting tool to track API/ABI changes. Although he did not really help in this particular case, I want to leave this link here: https://abi-laboratory.pro/tracker/timeline/nss/

@JustOff

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2018

It seems that the first method also indirectly forces NSS to use DBM, that means we still can allow using system-nss. I have to make another clean build to verify it is so.

Sorry, I was wrong.

@wolfbeast

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2018

The SQL path can't be taken unless Mozilla comes out and lets us know exactly what was changed in Firefox to use and enable NSS-SQL.
I've made a work branch and tagged commit with the DBM patch method for testing.

@JustOff

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2018

@JustOff

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2018

Unless we find a way to explicitly request to use DBM, by adding a type prefix to the database directory

Here is the alternative method to keep using DBM with NSS 3.35, based on bug 1398932.
It allows to build using --with-system-nss, as the changes made outside the NSS tree.

I'm going to switch my primary PM28 to the build made using this method to give it maximum testing.

@wolfbeast

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2018

JustOff's method is using the API available to us with a prefix and is the best solution (until, of course, Mozilla decides that dbm should be removed from NSS entirely)

@JustOff

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2018

Out of curiosity I created the build using nspr-4.19/nss-3.36.4 from esr60, and it looks like it also works well.

@wolfbeast

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2018

That would be a next step. First wanted to see if 3.35 could be made to work properly.

@mattatobin

This comment has been minimized.

Copy link
Member

commented Jun 6, 2018

I would hope the requesting DBM won't be removed because Linux but who can say.

@JustOff

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2018

Btw, don't we need to update NSPR_MINVER to the current 4.18?

@wolfbeast

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2018

I've not seen any further issues with the DBM: prefix in place. spun off the NSS update to a follow-up so we can close this as it's solved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.