CVE-2020-36732: Security issue · Issue #254 · brix/crypto-js

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

Closed

MarcinHoppe opened this issue

Jan 30, 2020

· 17 comments

Closed

Security issue #254

MarcinHoppe opened this issue

Jan 30, 2020

· 17 comments

Comments

I’m a member of the Node.js Foundation Ecosystem Security Working Group and we received a report of a vulnerability in this module.

We tried inviting the author by e-mail but received no response so I’m opening this issue and inviting anyone with commit and npm publish rights to collaborate with us on a fix.

I’ve received the mail.
But I did not create hacker one account for, I’ve to many accounts and I’m fed up with creating accounts over accounts for thing I do not use on a daily basis.

I do understand the security issue and it’s actually a pretty well known one.
I also know that this change in the past did not eliminate the issue completely.
ff1f003

There is a solution for modern JS environments we could go for, but it might break CryptoJS in a lot of other JS environments.

Right now, I just want to point out, I’m not ignoring it.
But on the other hand I can not say for sure when it’s getting fixed.

Would you mind if we disclosed the vulnerability to warn users of this flaw? It looks like this knowledge is already public.

@MarcinHoppe I just released a new version 3.2.0 which should fix this issue.

As mentioned the new version might not run in old environments like old IE … but it fixes the vulnerability for all who use the new version in modern environments.

@evanvosberg Many thanks! I will ask the original reporter to confirm this resolves the issue and proceed with the disclosure.

I will close this issue now as the fixed package has been released. Once again thank you for working on the fix and collaborating with us!

@MarcinHoppe The update released makes the issue even worse. The author of this commit understands nothing about cryptographically secure numbers.
His fix is to generate a random number like this:

randomFloat = float("0." + cryptoSecureRandomInt()); // Float generated with 3 bytes of entropy
random4Bytes = floor(randomFloat * 0x100000000);

He generates 4 “secure” bytes with 3 bytes of entropy. This is objectively worse than using Math.random() to generate random numbers.

Just from a quick test, we get that it’s very clear that there’s a clear bias:

let crypto = require(“crypto”);

let randomByte = function () { return (Number(“0.” + crypto.randomBytes(3).readUIntBE(0, 3)) * 0x100000000) | 0; };

let randomByte2 = function () { return crypto.randomBytes(4).readInt32LE(); };

let array = new Array(256); for (let i = 0; i < 256; ++i) { array[i] = 0; }

for (let i = 0; i < 1000000; ++i) { let n = randomByte(); array[n & 0xff]++; array[(n >> 8) & 0xff]++; array[(n >> 16) & 0xff]++; array[(n >> 24) & 0xff]++; }

console.log(“Using crypto-js method:”); for (let i = 0; i < 256; ++i) { console.log(i + ": " + array[i]); }

for (let i = 0; i < 256; ++i) { array[i] = 0; }

for (let i = 0; i < 1000000; ++i) { let n = randomByte2(); array[n & 0xff]++; array[(n >> 8) & 0xff]++; array[(n >> 16) & 0xff]++; array[(n >> 24) & 0xff]++; }

console.log(“Using sensible method:”); for (let i = 0; i < 256; ++i) { console.log(i + ": " + array[i]); }

Using crypto-js method:
0: 11827
1: 11569
2: 11395
3: 11569
4: 11539
5: 11666
6: 11618
7: 11624
8: 11703
9: 11566
10: 11585
11: 11671
12: 11645
13: 11495
14: 11530
15: 11682
16: 11754
17: 11709
18: 11762
19: 11844
20: 11705
21: 11840
22: 11597
23: 11825
24: 11770
25: 22133
26: 37537
27: 37338
28: 37604
29: 37222
30: 37574
31: 37632
32: 37651
33: 37440
34: 37839
35: 37828
36: 37472
37: 37554
38: 37453
39: 37482
40: 37594
41: 37386
42: 36186
43: 14334
44: 14372
45: 14135
46: 14230
47: 14314
48: 14295
49: 14228
50: 14343
51: 14341
52: 14068
53: 14481
54: 14190
55: 14236
56: 14393
57: 14164
58: 14160
59: 14265
60: 14362
61: 14284
62: 14178
63: 14390
64: 14270
65: 14327
66: 14347
67: 14351
68: 14110
69: 14276
70: 14313
71: 14483
72: 14450
73: 14305
74: 14213
75: 14260
76: 14193
77: 14346
78: 14293
79: 14241
80: 14315
81: 14318
82: 14079
83: 14355
84: 14062
85: 14345
86: 14496
87: 14211
88: 14580
89: 14247
90: 14117
91: 14285
92: 14240
93: 14299
94: 14500
95: 14167
96: 14592
97: 14189
98: 14236
99: 14353
100: 14298
101: 14328
102: 13897
103: 14433
104: 14146
105: 14361
106: 14409
107: 14353
108: 14267
109: 14301
110: 13998
111: 14262
112: 14135
113: 14236
114: 14199
115: 14323
116: 14242
117: 14405
118: 14072
119: 14388
120: 14318
121: 14476
122: 14522
123: 14459
124: 14475
125: 14402
126: 14364
127: 14357
128: 14387
129: 14294
130: 14315
131: 14243
132: 14146
133: 14328
134: 14233
135: 14219
136: 14215
137: 14282
138: 14236
139: 14366
140: 14381
141: 14195
142: 14419
143: 14337
144: 14247
145: 14303
146: 14399
147: 14404
148: 14247
149: 14276
150: 14461
151: 14307
152: 14300
153: 14338
154: 14336
155: 14331
156: 14292
157: 14209
158: 14231
159: 14304
160: 14313
161: 14491
162: 14352
163: 14398
164: 14238
165: 14381
166: 14056
167: 14456
168: 14298
169: 14465
170: 14344
171: 14402
172: 14408
173: 14377
174: 14398
175: 14266
176: 14494
177: 14284
178: 14450
179: 14302
180: 14275
181: 14250
182: 14426
183: 14133
184: 14494
185: 14324
186: 14392
187: 14669
188: 14231
189: 14404
190: 14384
191: 14434
192: 14553
193: 14451
194: 14382
195: 14241
196: 14609
197: 14251
198: 14260
199: 14324
200: 14398
201: 14357
202: 14188
203: 14191
204: 14378
205: 14475
206: 14432
207: 14356
208: 14227
209: 14453
210: 14522
211: 14195
212: 14298
213: 14429
214: 14359
215: 14230
216: 14061
217: 14368
218: 14198
219: 14398
220: 14633
221: 14087
222: 14134
223: 14451
224: 14424
225: 14135
226: 14231
227: 14473
228: 14347
229: 14338
230: 14215
231: 14365
232: 14340
233: 14495
234: 14277
235: 14273
236: 14498
237: 14450
238: 14420
239: 14415
240: 14331
241: 14395
242: 14349
243: 14276
244: 14003
245: 14299
246: 14391
247: 14361
248: 14400
249: 14225
250: 14350
251: 14415
252: 14153
253: 14336
254: 14424
255: 14169
Using sensible method:
0: 15358
1: 15855
2: 15919
3: 15687
4: 15621
5: 15381
6: 15682
7: 15528
8: 15394
9: 15723
10: 15737
11: 15393
12: 15595
13: 15547
14: 15403
15: 15675
16: 15538
17: 15753
18: 15669
19: 15848
20: 15591
21: 15638
22: 15697
23: 15812
24: 15457
25: 15574
26: 15444
27: 15535
28: 15409
29: 15540
30: 15638
31: 15740
32: 15609
33: 15641
34: 15502
35: 15650
36: 15690
37: 15680
38: 15685
39: 15568
40: 15572
41: 15849
42: 15586
43: 15549
44: 15724
45: 16009
46: 15542
47: 15745
48: 15832
49: 15682
50: 15807
51: 15568
52: 15539
53: 15484
54: 15760
55: 15674
56: 15480
57: 15847
58: 15654
59: 15581
60: 15607
61: 15719
62: 15491
63: 15413
64: 15341
65: 15800
66: 15642
67: 15676
68: 15671
69: 15678
70: 15474
71: 15653
72: 15716
73: 15566
74: 15559
75: 15577
76: 15773
77: 15521
78: 15528
79: 15657
80: 15561
81: 15871
82: 15590
83: 15669
84: 15621
85: 15826
86: 15496
87: 15444
88: 15599
89: 15416
90: 15662
91: 15650
92: 15626
93: 15469
94: 15592
95: 15593
96: 15907
97: 15498
98: 15345
99: 15669
100: 15303
101: 15554
102: 15606
103: 15566
104: 15682
105: 15591
106: 15473
107: 15694
108: 15647
109: 15831
110: 15716
111: 15613
112: 15502
113: 15943
114: 15605
115: 15706
116: 15777
117: 15453
118: 15466
119: 15830
120: 15608
121: 15693
122: 15480
123: 15809
124: 15756
125: 15795
126: 15783
127: 15737
128: 15647
129: 15737
130: 15446
131: 15700
132: 15619
133: 15533
134: 15691
135: 15668
136: 15634
137: 15729
138: 15482
139: 15478
140: 15680
141: 15348
142: 15444
143: 15476
144: 15540
145: 15724
146: 15364
147: 15535
148: 15586
149: 15614
150: 15650
151: 15758
152: 15732
153: 15632
154: 15735
155: 15715
156: 15628
157: 15672
158: 15520
159: 15346
160: 15485
161: 15685
162: 15697
163: 15554
164: 15434
165: 15641
166: 15737
167: 15780
168: 15808
169: 15527
170: 15547
171: 15719
172: 15621
173: 15798
174: 15747
175: 15522
176: 15631
177: 15578
178: 15670
179: 15671
180: 15686
181: 15697
182: 15619
183: 15422
184: 15571
185: 15846
186: 15848
187: 15541
188: 15736
189: 15784
190: 15550
191: 15594
192: 15602
193: 15661
194: 15754
195: 15658
196: 15533
197: 15520
198: 15633
199: 15488
200: 15528
201: 15636
202: 15491
203: 15711
204: 15858
205: 15502
206: 15501
207: 15493
208: 15678
209: 15348
210: 15580
211: 15583
212: 15996
213: 15908
214: 15491
215: 15710
216: 15566
217: 15641
218: 15751
219: 15613
220: 15516
221: 15567
222: 15887
223: 15887
224: 15656
225: 15859
226: 15582
227: 15739
228: 15778
229: 15610
230: 15496
231: 15366
232: 15398
233: 15590
234: 15598
235: 15688
236: 15755
237: 15492
238: 15739
239: 15755
240: 15689
241: 15569
242: 15306
243: 15787
244: 15534
245: 15517
246: 15768
247: 15563
248: 15595
249: 15770
250: 15518
251: 15473
252: 15592
253: 15650
254: 15855
255: 15338

I know cryptographically secure random number generation is a difficult subject, that’s why I asked the original security researcher to review the fix before disclosure. I included a link to this comment in the HackerOne report.

This was the base

            for (var i = 0; i < nBytes; i += 4) {
                words.push((Math.random() * 0x100000000) | 0);
            }

As Math.random() generates a float number between 0 and 1 , it was just replace by method generator float numbers as well but using the native crypto module.
Everything around (…* 0x100000000) | 0 has not been touched by this.

@wartab you are welcome to provide a better approach.

I provided a better approach in that comment already. If you are unable to understand that, I recommend people to jump ship and find a library where the authors have a better understanding of cryptography.

I just explained, what changed.

I did not say I do not understand what you wrote.

A typical way of contribution is creating pull requests, that’s what I meant by

@wartab you are welcome to provide a better approach.

Well I can take code out of the comment and implement it, that’s fine as well.

Right now, I do not have free time. So can get back in this in some hours.

The simple fact you even thought about generating random bytes this way is bonkers.

You generate 4 random bytes using integers generated with 3 random bytes.
You do not understand how floating point numbers are internally structured.

This is extremely hazardous and I recommend you read some literature about cryptography before continuing to actively maintain the code source of this repository.

How does shouting and blaming help to solve the issue?

Are you interested in a solution or just in blaming?

First step: remove the latest release, it’s buggy anyway, as can be seen in another issue.
I’d rather have people generate random numbers with a difficult to exploit vulnerability than generating random numbers with a completely biased methodology.

I’ll remove the last release.

generate 4 random bytes using integers generated with 3 random bytes.

This does not make any sense, that’s pretty clear. Just imagine that people can overlook typos again and again. That’s how bugs often come along …

Later I’ll create a new branch working on a fix, would you be up to review/support @wartab?

What does this have to do with a typo? You should not concatenate “0.” and some random int to generate random numbers.

I beg you, look into how floating point numbers are represented in a computer.

Use my randomByte2() function to generate 4 random bytes instead of what you do in randomByte(). That’s all you need to do.
Don’t multiply a float number to generate random bytes when both the browser and nodejs already provide you with functions to generate random bytes.

Also given this change will introduce a breaking change, as it will break in older browsers, I believe the new version should be a major version bump rather than a minor version.

Also yes, once you removed the release, I’ll be fine checking whatever you release in that regard before publishing.

The package could not be unpublished but it’s deprecated now.

I’ve created a new branch.

@wartab feel free to comment on the pull request #257