1449169 - NSS should assume SQlite >= 3.5.0 and support read-only database opening using the sqlite3_open_v2 API

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P3)

Description

[Kai Engert (:KaiE:)]

An application initializes NSS with the following call.

  NSS_InitContext(nsscertdir, prefix, prefix, SECMOD_DB,
                  &initparams, NSS_INIT_READONLY);

The NSS SQL database is used, which uses SQLite.

Despite the request to open read-only, we see attempts to open the files in write mode.

We haven't yet debugged which code causes the attempt to open in write mode.

Comment 1

[Kai Engert (:KaiE:)]

Before we debug this inside NSS, we'd like to ask:

Could it be a possible cause, that SQLite always attempt to open its files in write mode, potentially for locking purposes?

Comment 2

[D. Richard Hipp]

Which SQLite files are we talking about?  The main database file?

SQLite should be attempting to open main database file read-only if you use the SQLITE_OPEN_READONLY flag on sqlite3_open_v2().  Are you seeing something difference? How exactly are you trying to open the database?  Can you provide additional context?

Comment 3

[Kai Engert (:KaiE:)]

Thanks for the initial feedback, which makes it clear that we likely cannot solve this without debugging what's really going on at the NSS level.

Comment 4

[Kai Engert (:KaiE:)]

NSS never calls sqlite3_open_v2.

Inside the NSS internal function sdb_openDB, NSS doesn't make any use of the read-only flag that was provided as a parameter (flags == 1 == SDB_RDONLY).

That function has the following comment:
    /*
     * in sqlite3 3.5.0, there is a new open call that allows us
     * to specify read only. Most new OS's are still on 3.3.x (including
     * NSS's internal version and the version shipped with Firefox).
     */

Bob, it appears that because of the situation described in the above comment, which was written at the time of the original implementation, NSS never uses read-only access when opening sqlite database files.

We'd have to change NSS to use the new sqlite3_open_v2.

I see that even on RHEL 6, we already have SQLite 3.6.20 available, so I think it should be acceptable to change NSS.

Comment 5

[D. Richard Hipp]

SQLite 3.6.20 is quite old.  The latest releases of SQLite are more than 3x faster and contain security fixes.  Can you statically link against SQLite (as FF does) and hence always have the latest code?

Comment 6

[Kai Engert (:KaiE:)]

No, on Linux distributions we commonly use shared linking. This allows us to reduce the OS code size, and in times of security incidents, fewer places must be updated.

Comment 7

[Hubert Kario]

Old version of SQLite (or for that matter, any package) on RHEL don't mean that they lack security fixes, it just specifies the oldest code that was shipped as part of upstream release – more or less the API of the package. The code needs to link to system libraries.

Please see https://access.redhat.com/security/updates/backporting and https://fedoraproject.org/wiki/Bundled_Libraries for background info.

Comment 8

[D. Richard Hipp]

Using a test program, I have confirmed that version 3.6.20 of SQLite does honor the SQLITE_OPEN_READONLY flag to sqlite3_open_v2(), causing it to open the database file using open(..., O_RDONLY).  (I only tested unix - presumably it works on Windows too.)  So I think this takes us back to the question of:  What SQLite API is NSS using to open the database?

Comment 9

[Kai Engert (:KaiE:)]

Hello Richard, I'm sorry if I haven't been clear in the previous statements.

The fault is inside NSS. NSS never uses the v2 API. NSS never asks SQLite to open read-only.
Based on that, I suggest to change NSS to start using the v2 API.

The question is, does this change align with the backwards compatibility requirements of NSS.
In the past, compatibility with SQLite 3.3 was required.

I believe we can increase NSS' minimal SQLite version expectation to 3.6.20, and change NSS to use the v2 API.

Comment 10

[Kai Engert (:KaiE:)]

We intend to upgrade the source code snapshot distributed with NSS to verion 3.6.20 (the currently oldest supported version), and change NSS to use the v2 open API, and support read only opening.

Comment 11

[Kai Engert (:KaiE:)]

Bob has suggested, as part of the work on this bug, we should check if NSS has any #ifdef based on dynamically detected sqlite version, and possible cleaning up those we can.

Comment 12

[Robert Relyea]

It think it's fine to use the 3.6.20 API now. When the original code went in, there were still users on the older API. The embedded sqlite that NSS uses is quite old, not not really updated (unlike the frozen version in RHEL 6), so statically linking is definitely out. Most NSS users supply their own sqlite (either through the OS or in their application), and those versions are likely to be more up to date than the one embedded in the NSS source tree (which is really there just to allow NSS to build and test stand alone).

Comment 13

[Kai Engert (:KaiE:)]

Attachment: 1449169-v1.patch

Comment 14

[Kai Engert (:KaiE:)]

try build running here:
https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=bc71ec59c3b41f01adfbd417d19f8f2e51908ec6

I'd like to do the update of the bundled sqlite snapshot in a separate step.

Comment 15

[Kai Engert (:KaiE:)]

(In reply to Kai Engert (:kaie:) from comment #14)
> I'd like to do the update of the bundled sqlite snapshot in a separate step.

Unnecessary, we already bundle the newer version 3.10.2 (bug 1234698).

Comment 16

[Hubert Kario]

Comment on attachment 8967710 [details] [diff] [review]
1449169-v1.patch

shouldn't the minimal expected version be changed also by this patch?
or it's not a check run by code/doesn't live in nss repo?

Comment 17

[Kai Engert (:KaiE:)]

I don't think there's NSS code that checks for the version. But I'll doublecheck.

Comment 18

[Kai Engert (:KaiE:)]

(we didn't update any version checks in the past, when we updated the bundled copy to newer versions, which suggests that there is no such check)

Comment 19

[Daiki Ueno [:ueno]]

Comment on attachment 8967710 [details] [diff] [review]
1449169-v1.patch

Review of attachment 8967710 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/softoken/sdb.c
@@ +648,4 @@
>      /*
>       * in sqlite3 3.5.0, there is a new open call that allows us
>       * to specify read only. Most new OS's are still on 3.3.x (including
>       * NSS's internal version and the version shipped with Firefox).

I would suggest to update this comment reflecting the current situation (3.6.20 is required now and 3.10.2 is included).

Comment 20

[Kai Engert (:KaiE:)]

Thanks. Version 3.6.20 was mentioned because it's probably the oldest version we currently have to support. But the newest APIs we're using were from version 3.5.0. Increasing the minimal version requirement to 3.5.0 should be sufficient. We should mention that in the NSS release notes for this bugfix.

I'll remove the above comment, and add a comment mentioning the 3.5.0 requirement.

Comment 21

[Kai Engert (:KaiE:)]

Attachment: 1449169-v2.patch

Comment 22

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/projects/nss/rev/e7ccf89f73a2