feat(compartment-mapper): Consistent hashing #799

kriskowal · 2021-06-26T00:04:30Z

Fixes #794

See change to NEWS.md for details.

katelynsills · 2021-06-30T16:42:51Z

Missed this review request, sorry! Will tackle this today

kriskowal · 2021-06-30T21:08:09Z

Missed this review request, sorry! Will tackle this today

You didn’t miss this! I added you yesterday. I’ve been fishing for a reviewer who is not buried under other priorities!

katelynsills

I would need to know more about compartment-mapper to review properly, but here's some questions that occurred to me.

katelynsills · 2021-06-30T22:50:51Z

packages/compartment-mapper/test/test-main.js

+    async () => {
+      await application.import({


Suggested change

async () => {

await application.import({

() => application.import({

Just curious, any reason not to do it this way?

I think that would be a valid simplification.

katelynsills · 2021-06-30T22:53:47Z

packages/compartment-mapper/test/test-main.js

+    },
+  );
+
+  const application = await parseArchive(archive, 'app.agar', {


Could the bad hash fail in parseArchive? Seems strange to have to put together my globals and other settings to fail later on the hash being bad. I don't know much about how this works, but it seems like it'd be nice to fail faster.

What makes the hash bad isn’t so much that it’s inaccurate as that it doesn’t match the one given to the loader in the later steps.

All of the functions that accept a sha512 power could do a trivial check, like computeSha512(new Uint8Array(0)) and verify that it returns the correct SHA-512 for that or a smattering of known byte sequences. Then it would be possible to fail on parseArchive, but, on the other hand, then it wouldn’t be possible to create an invalid archive with parseArchive and test the loader’s ability to recognize that it is invalid.

katelynsills · 2021-06-30T22:59:53Z

packages/compartment-mapper/test/test-main.js

+  const { computeSha512 } = readPowers;
+
+  const application = await parseArchive(archive, 'app.agar', {
+    computeSha512,


Hmm, so the desire to check the hash is communicated solely by passing in the capability to check the hash? If I were auditing this, it seems like it is not as clear as it could be, but I guess otherwise you'd have to pass a setting and the authority.

Aye. The alternative is to have a redundant option. I’m not entirely sure which is better.

warner

Overall approach sounds good, but I'd like to make the API as safe to use as possible.

warner · 2021-07-14T06:13:23Z

packages/compartment-mapper/NEWS.md

+    every file is consistent.
+  - `importArchive`, `loadArchive`, and `parseArchive` all optionally accept a
+    `computeSha512` capability, use it to verify the integrity of the archive
+    and provide the hash of the contained `compartment-map.json` so the caller


Two concerns:

Do the load/execute functions return the computed hash? I think that's too late: the wrong code has already had a chance to execute with whatever authorities you passed in. A safer API would be to pass the expected hash in, have the compartment-map.json loader hash what it loads, assert that it matches the expected value, and only proceed if it matches. "No evaluation of unverified code". At each point we're about to load a file, we should know the expected hash ahead of time, and we don't convert the inert bytes into behavior until after it's passed the check.

It sounds like the contents of the compartment map determines whether or not a hash is checked on each module. That sounds reasonable, but we must make sure that this doesn't enable an attacker to substitute a doctored map (lacking module hashes) to disable any checks. I think the hash check of the map itself is sufficient to prevent this (they can't remove the module hashes and still meet the expected map hash), but "in-band" security markers that have broken many others systems (the JWT "alg": "none" field comes to mind).

This is a good catch and I’m revising the importArchive, loadArchive, and parseArchive functions to accept expectedSha512 which will get checked against the archived compartment-map.json.

If you pass computeSha512, every module must have a sha512 and pass an integrity check, regardless of whether you also provide expectedSha512.

warner · 2021-07-14T06:18:16Z

packages/compartment-mapper/src/import-hook.js

+          let sha512;
+          if (computeSha512 !== undefined) {
+            sha512 = computeSha512(transformedBytes);
+            if (expectedSha512 !== undefined && sha512 !== expectedSha512) {


Why is the expected hash optional here, when the earlier check (in makeImportHook) makes it mandatory (both in the case where the ability to hash at all has been provided)?

This is also a good catch. The expectedSha512 is in fact always undefined because this import hook maker only gets used when creating a compartment map from scratch, so we don’t need the code above to extract expectedSha512 (never exists) nor the code below to test it, just this bit in the middle to compute a new one if the power is provided.

warner · 2021-07-14T06:24:50Z

packages/compartment-mapper/test/test-main.js

+        Compartment,
+      }),
+    {
+      message: /failed a SHA-512 integrity check/,


Is it possible to test a hash mismatch in both the top-level compartment map, and in one of the modules being loaded, separately? Maybe by making the doctored hasher insert the corruption on the first call but not the later ones (and knowing exactly how many time the hasher is called during assembly). That's probably easier than creating a valid archive, taking it apart, flipping a bit in a hash, then reassembling it.

kriskowal · 2021-07-17T01:37:20Z

@warner Addressed feedback in b138baa:

The archive reading functions now test expectedSha512 before compute can occur. If you provided expectedSha512, you must provide computeSha512. This is instead of returning the sha512 on the application object, to drive home the intended safe usage pattern.
Fixed a mistake where computeSha512 was passed to link, which doesn’t do anything with that option.
Removed extraneous hash check code for routines that create a compartment map from whole cloth. That was dead code.
Altered the corruption test to corrupt only one file. That should be sufficient.
Added a test that deliberately corrupts the compartment-map.json of an archive in place, using the underlying zip file utility to do the corrupting. The test exercises the express case of a corrupt compartment map, as opposed to a corrupt module.

warner · 2021-07-18T00:38:43Z

packages/compartment-mapper/src/import-archive.js

+            );
+          }
+        } else {
+          throw new Error(


If I'm reading this correctly, if you 1: provide computeSha512 into the archive importer and 2: the compartment map did not specify a hash for the module, then the import will fail. I'm all for things that encourage hashes, including throwing random errors if you choose to omit them, but this smells of an inconsistency.

Providing the expected top-level compartment-map hash is a great signal to say that you care about the integrity of the thing you're loading.

Providing that hash without also providing the means to compute hashes is a clear mistake.

If you don't care about integrity, you wouldn't provide an expected compartment-map hash, but you might still provide the computeSha512 tool, just like you'd unconditionally provide the file-reading tool.

An archive which cares about integrity will include module hashes.

.. but maybe we're providing for archives which don't care about integrity, and lack those module hashes?

An archive whose compartment-map does not match the expected hash is a clear error.

So I see three questions:

If you provide computeSha512, but the archive does not include module hashes, I think this switch might fail.. should the application author (who may or may not have provided an expected compartment-map hash) think "oh, this archive should care about integrity more", or should they think "I shouldn't provide computeSha512" ?

What should an application which cares about integrity do when asked to load an archive which does not?

Should we even support the idea of an archive which does not care about integrity?

I think I like best:

If you provide computeSha512, regardless of whether you provide expectedSha512, every module in compartment-map.json that provides a hash will be checked. Hashes are not required.

If you provide computeSha512 and expectedSha512, the integrity of the compartment-map.json hash will be checked.

If you provide computeSha512 but do not provide expectedSha512, the hash of compartment-map.json will not be checked.

If you do not provide computeSha512 and do provide expectedSha512, this is a usage error.

If you provide neither expectedSha512 nor computeSha512, no hashes will be checked, regardless of whether the compartment map has any hashes.

warner · 2021-07-18T00:40:10Z

packages/compartment-mapper/src/import-archive.js

@@ -101,6 +144,20 @@ export const parseArchive = async (archiveBytes, archiveLocation) => {
  // TODO validate compartmentMap instead of leaning hard on the above type
  // assertion.

+  if (expectedSha512 !== undefined) {


Is it possible to move the hash-equals-expected test earlier, just after we get compartmentMapBytes and before we do anything else with it? That would be the most hygenic.

Sure. We can move the check before parsing the content of the compartment map.

warner

Small changes suggested, but if you didn't do them I'd still approve for landing.

kriskowal changed the title ~~feat(compartment-mapper): Consistent hashing (fixes #794)~~ feat(compartment-mapper): Consistent hashing Jun 26, 2021

kriskowal requested a review from warner June 26, 2021 00:05

kriskowal force-pushed the 794-consistent-hash branch 2 times, most recently from 2bdf518 to 72f776e Compare June 26, 2021 00:19

kriskowal changed the base branch from master to 794-text-fixture-maker June 26, 2021 00:19

kriskowal force-pushed the 794-text-fixture-maker branch from 103315e to 83e2894 Compare June 26, 2021 00:21

kriskowal force-pushed the 794-consistent-hash branch from 72f776e to 44c653f Compare June 26, 2021 00:21

kriskowal mentioned this pull request Jun 26, 2021

Endo archive hashing #794

Closed

kriskowal force-pushed the 794-consistent-hash branch from 44c653f to d304485 Compare June 26, 2021 00:28

kriskowal force-pushed the 794-text-fixture-maker branch from 2aae715 to 6d6f086 Compare June 26, 2021 00:29

kriskowal force-pushed the 794-consistent-hash branch 6 times, most recently from ef8c147 to e0eed22 Compare June 26, 2021 04:59

kriskowal force-pushed the 794-text-fixture-maker branch from 6d6f086 to 68f9289 Compare June 26, 2021 05:00

kriskowal force-pushed the 794-consistent-hash branch from e0eed22 to 1c4fa11 Compare June 26, 2021 05:00

kriskowal requested review from katelynsills and erights June 29, 2021 01:51

kriskowal removed the request for review from katelynsills June 30, 2021 23:18

katelynsills reviewed Jun 30, 2021

View reviewed changes

kriskowal force-pushed the 794-text-fixture-maker branch from 68f9289 to 383b914 Compare July 1, 2021 00:41

kriskowal force-pushed the 794-consistent-hash branch from 1c4fa11 to 2a96277 Compare July 1, 2021 00:41

kriskowal force-pushed the 794-text-fixture-maker branch from 383b914 to b9bed57 Compare July 1, 2021 00:51

kriskowal force-pushed the 794-consistent-hash branch from 2a96277 to a48da5f Compare July 1, 2021 00:51

kriskowal force-pushed the 794-text-fixture-maker branch from b9bed57 to 6fb41a8 Compare July 1, 2021 01:01

kriskowal force-pushed the 794-consistent-hash branch from a48da5f to 285a29a Compare July 1, 2021 01:01

kriskowal force-pushed the 794-consistent-hash branch 2 times, most recently from d2c100f to 65f2986 Compare July 1, 2021 21:49

Base automatically changed from 794-text-fixture-maker to master July 1, 2021 22:31

warner requested changes Jul 14, 2021

View reviewed changes

kriskowal force-pushed the 794-consistent-hash branch 2 times, most recently from b370ead to b138baa Compare July 17, 2021 01:28

kriskowal requested a review from warner July 17, 2021 01:37

warner reviewed Jul 18, 2021

View reviewed changes

warner approved these changes Jul 18, 2021

View reviewed changes

kriskowal added 2 commits July 21, 2021 16:17

feat(compartment-mapper): Consistent hashing

fba461f

fix(endo): Sync with deps

f714c25

kriskowal force-pushed the 794-consistent-hash branch from b138baa to f714c25 Compare July 21, 2021 23:17

kriskowal merged commit 6e9243e into master Jul 21, 2021

kriskowal deleted the 794-consistent-hash branch July 21, 2021 23:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(compartment-mapper): Consistent hashing #799

feat(compartment-mapper): Consistent hashing #799

kriskowal commented Jun 26, 2021 •

edited

Loading

katelynsills commented Jun 30, 2021

kriskowal commented Jun 30, 2021

katelynsills left a comment

katelynsills Jun 30, 2021

kriskowal Jul 1, 2021

katelynsills Jun 30, 2021

kriskowal Jul 1, 2021

katelynsills Jun 30, 2021

kriskowal Jul 1, 2021

warner left a comment

warner Jul 14, 2021

kriskowal Jul 17, 2021

warner Jul 14, 2021

kriskowal Jul 17, 2021

warner Jul 14, 2021

kriskowal commented Jul 17, 2021

warner Jul 18, 2021

kriskowal Jul 21, 2021

warner Jul 18, 2021

kriskowal Jul 21, 2021

warner left a comment

	async () => {
	await application.import({
	() => application.import({

feat(compartment-mapper): Consistent hashing #799

feat(compartment-mapper): Consistent hashing #799

Conversation

kriskowal commented Jun 26, 2021 • edited Loading

katelynsills commented Jun 30, 2021

kriskowal commented Jun 30, 2021

katelynsills left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

warner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kriskowal commented Jul 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

warner left a comment

Choose a reason for hiding this comment

kriskowal commented Jun 26, 2021 •

edited

Loading