Use FIPS-approved SHA256 instead of weak MD5 #8379

azatsarynnyy · 2020-08-13T17:30:55Z

Signed-off-by: Artem Zatsarynnyi azatsary@redhat.com

What it does

Fixes #8378 by removing MD5 usage.

How to test

The simplest variant to test it is to run Theia and see that there's no unusual error logged, frontend is loaded as usual and Task of types process/shell works as expected.

If it's possible, you can check the same in an OS with FIPS mode enabled. E.g. here's the instructions for Red Hat Enterprise Linux 7.

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

paul-marechal

LGTM!

akosyakov · 2020-08-14T04:35:10Z

Is it a breaking change for all workspace and plugin storages out there? All data will be lost? We need a migration?

akosyakov · 2020-08-14T05:07:00Z

I've noticed also that we bogusly encode workspace roots into workspace id leading to bugs like #7472

I think the proper solution will be to align with VS Code implementation, maybe it does not even require usage of crypto. There is not a real security concern in affected code. It seems to be misusage of crypto to generate identifiers. Plus we should make sure that data are not lost, but migrated.

akosyakov · 2020-08-14T05:18:17Z

packages/plugin-ext/src/plugin/types-impl.ts

@@ -1457,7 +1457,7 @@ export class ProcessExecution {
    }

    public computeId(): string {
-        const hash = crypto.createHash('md5');
+        const hash = crypto.createHash('sha256');


Not sure about backward computability consequences of changing this file. Do we store them somewhere and should read again or it is pure runtime thing?

As I see, it's used at runtime only.
BTW I've found that it has been reworked in VS Code to generate IDs based on the command line instead of hashes.
microsoft/vscode@36d9d04
We need to update it in Theia correspondingly. I'll take a look at whether it can be done within this PR.

I've changed the IDs generation according to the current VS Code state. Now, it doesn't require crypto Node module.

azatsarynnyy · 2020-08-19T16:16:10Z

It breaks the Tasks list.

With the changes:

From master branch:

Also, NPM SCRIPTS view is empty:

Need to investigate how it's related.

azatsarynnyy · 2020-09-02T16:12:18Z

I can reproduce both issues (mentioned above) on master branch as well. They are randomly reproducible and are not related to the PR changes.

benoitf · 2020-09-03T07:48:13Z

I'm restarting travis job that failed on macos

akosyakov · 2020-09-03T09:02:24Z

@azatsarynnyy it is good, but we cannot switch algorithm for global and plugin state without providing migrations for old state. It will wipe out all state for VS Code extensions for existing installations.

azatsarynnyy · 2020-09-03T13:34:32Z

@azatsarynnyy it is good, but we cannot switch algorithm for global and plugin state without providing migrations for old state. It will wipe out all state for VS Code extensions for existing installations.

@akosyakov I agree that we should migrate the plugins' data.
I think we can keep the migration code for one release (1.6.0) with a note in the changelogs. And then remove the migration and MD5 usage in the next release - 1.7.0. WDYT?

akosyakov · 2020-09-03T14:53:58Z

I think we can keep the migration code for one release (1.6.0) with a note in the changelogs. And then remove the migration and MD5 usage in the next release - 1.7.0. WDYT?

What would happen if someone migrated from 1.0.0 to 1.8.0? user data will be erased? Probably it is just should stay in codebase forever in the same way since it touches end user data.

azatsarynnyy · 2020-09-04T07:43:50Z

I think we can keep the migration code for one release (1.6.0) with a note in the changelogs. And then remove the migration and MD5 usage in the next release - 1.7.0. WDYT?

What would happen if someone migrated from 1.0.0 to 1.8.0? user data will be erased? Probably it is just should stay in codebase forever in the same way since it touches end user data.

You are right @akosyakov. Thanks. I'll look for another way to fix #8378
Probably, we need to detect somehow whether we're running in a FIPS environment and then use SHA-256 instead of MD5.

akosyakov · 2020-09-07T15:06:40Z

packages/plugin-ext/src/main/node/paths/plugin-paths-service.ts

+            // md5 is not FIPS-approved but we have to continue use it as there're existing storage folders based on it
+            return crypto.createHash('md5').update(str).digest('hex');
+        } catch (e) {
+            // FIPS-compliant


Can we narrow down the error and log in all other cases? to avoid false positively creating sha256 hashes?

Signed-off-by: Artem Zatsarynnyi <azatsary@redhat.com>

akosyakov

code wise looks good to me, but someone has to try it out

azatsarynnyy · 2020-09-14T07:47:04Z

Here are several screenshots made in the downstream product.
With FIPS mode enabled, storage folders names are generated with SHA256

With FIPS mode disabled, storage folders names are generated with MD5 - with no changes comparing to master branch

Shell task is running well in both modes, without any changes

If there're no objections, I'll merge it by the EOD.

azatsarynnyy requested review from akosyakov and benoitf August 13, 2020 17:31

paul-marechal approved these changes Aug 13, 2020

View reviewed changes

vince-fugnitto added the security issues related to security label Aug 13, 2020

akosyakov added the vscode issues related to VSCode compatibility label Aug 14, 2020

akosyakov reviewed Aug 14, 2020

View reviewed changes

azatsarynnyy changed the title ~~Use FIPS-approved SHA256 instead of weak MD5~~ [WIP] Use FIPS-approved SHA256 instead of weak MD5 Sep 2, 2020

akosyakov reviewed Sep 7, 2020

View reviewed changes

azatsarynnyy added 2 commits September 9, 2020 18:19

Update Tasks IDs generation to avoid using crypto node module

f071134

Signed-off-by: Artem Zatsarynnyi <azatsary@redhat.com>

Make plugin storage path FIPS-compliant

604de71

Signed-off-by: Artem Zatsarynnyi <azatsary@redhat.com>

azatsarynnyy force-pushed the az/crypto branch from f23730d to 604de71 Compare September 9, 2020 15:20

akosyakov approved these changes Sep 10, 2020

View reviewed changes

azatsarynnyy merged commit 4378748 into master Sep 14, 2020

azatsarynnyy deleted the az/crypto branch September 14, 2020 19:04

azatsarynnyy changed the title ~~[WIP] Use FIPS-approved SHA256 instead of weak MD5~~ Use FIPS-approved SHA256 instead of weak MD5 Sep 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use FIPS-approved SHA256 instead of weak MD5 #8379

Use FIPS-approved SHA256 instead of weak MD5 #8379

azatsarynnyy commented Aug 13, 2020 •

edited

Loading

paul-marechal left a comment

akosyakov commented Aug 14, 2020 •

edited

Loading

akosyakov commented Aug 14, 2020 •

edited

Loading

akosyakov Aug 14, 2020

azatsarynnyy Aug 19, 2020

azatsarynnyy Sep 2, 2020

azatsarynnyy commented Aug 19, 2020

azatsarynnyy commented Sep 2, 2020

benoitf commented Sep 3, 2020

akosyakov commented Sep 3, 2020

azatsarynnyy commented Sep 3, 2020

akosyakov commented Sep 3, 2020 •

edited

Loading

azatsarynnyy commented Sep 4, 2020

akosyakov Sep 7, 2020

azatsarynnyy Sep 9, 2020

akosyakov left a comment

azatsarynnyy commented Sep 14, 2020

Use FIPS-approved SHA256 instead of weak MD5 #8379

Use FIPS-approved SHA256 instead of weak MD5 #8379

Conversation

azatsarynnyy commented Aug 13, 2020 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

paul-marechal left a comment

Choose a reason for hiding this comment

akosyakov commented Aug 14, 2020 • edited Loading

akosyakov commented Aug 14, 2020 • edited Loading

akosyakov Aug 14, 2020

Choose a reason for hiding this comment

azatsarynnyy Aug 19, 2020

Choose a reason for hiding this comment

azatsarynnyy Sep 2, 2020

Choose a reason for hiding this comment

azatsarynnyy commented Aug 19, 2020

azatsarynnyy commented Sep 2, 2020

benoitf commented Sep 3, 2020

akosyakov commented Sep 3, 2020

azatsarynnyy commented Sep 3, 2020

akosyakov commented Sep 3, 2020 • edited Loading

azatsarynnyy commented Sep 4, 2020

akosyakov Sep 7, 2020

Choose a reason for hiding this comment

azatsarynnyy Sep 9, 2020

Choose a reason for hiding this comment

akosyakov left a comment

Choose a reason for hiding this comment

azatsarynnyy commented Sep 14, 2020

azatsarynnyy commented Aug 13, 2020 •

edited

Loading

akosyakov commented Aug 14, 2020 •

edited

Loading

akosyakov commented Aug 14, 2020 •

edited

Loading

akosyakov commented Sep 3, 2020 •

edited

Loading