Opened 2 months ago

Closed 2 weeks ago

Last modified 2 weeks ago

#31383 closed defect (invalid)

OpenSSL CVE-2019-1552

Reported by: cypherpunks Owned by: tbb-team
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Major Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Child Tickets

Change History (18)

comment:1 Changed 2 months ago by gk

Component: - Select a componentApplications/Tor Browser
Owner: set to tbb-team
Status: newneeds_information

What is the attack scenario here? Note, we _do_ set --prefix.

comment:2 Changed 2 months ago by cypherpunks

"The one with a vulnerable-on-Windows *nix path"?

comment:3 Changed 4 weeks ago by cypherpunks

What is the attack scenario here?

ticket:13359#comment:1

comment:5 Changed 3 weeks ago by gk

Resolution: fixed
Status: needs_informationclosed

Fixed by #31844.

comment:6 Changed 3 weeks ago by cypherpunks

Resolution: fixed
Status: closedreopened

No, it's not fixed. Program Files (x86) looks even like the same hole for 32-bit Windows. Fixing compilation doesn't mean fixing a CVE. Anyway, that's for the default fallback only.

Your scenario is different, because you ship OpenSSL with a portable application, which is known as an app-local installation. That's why you are not allowed to use the default paths of system-wide OpenSSL. You have been warned about that in ticket:23396#comment:14, but still can't realize what it means, it seems :(

If you read the wiki above, you would know that you should use a "rule of thumb" and set --prefix/--openssldir properly. But assuming that the Tor Browser's directory is still user-writable in most installations :(, what paths should be used as safe? C:\Windows (or even %WINDIR%, if supported?) or some path in it? What is the consensus here?

comment:7 in reply to:  6 Changed 3 weeks ago by gk

Status: reopenedneeds_information

Replying to cypherpunks:

No, it's not fixed. Program Files (x86) looks even like the same hole for 32-bit Windows. Fixing compilation doesn't mean fixing a CVE. Anyway, that's for the default fallback only.

Your scenario is different, because you ship OpenSSL with a portable application, which is known as an app-local installation. That's why you are not allowed to use the default paths of system-wide OpenSSL. You have been warned about that in ticket:23396#comment:14, but still can't realize what it means, it seems :(

Not sure what you mean. This is what we get for 64bit Windows:

001f5900: 4f50 454e 5353 4c44 4952 3a20 2243 3a2f  OPENSSLDIR: "C:/
001f5910: 5072 6f67 7261 6d20 4669 6c65 732f 436f  Program Files/Co
001f5920: 6d6d 6f6e 2046 696c 6573 2f53 534c 2200  mmon Files/SSL".
001f5930: 454e 4749 4e45 5344 4952 3a20 2243 3a2f  ENGINESDIR: "C:/
001f5940: 5072 6f67 7261 6d20 4669 6c65 732f 4f70  Program Files/Op
001f5950: 656e 5353 4c2f 6c69 622f 656e 6769 6e65  enSSL/lib/engine
001f5960: 732d 315f 3122 0000 6275 696c 7420 6f6e  s-1_1"..built on

and 32bit

001db520: 6777 0000 4f50 454e 5353 4c44 4952 3a20  gw..OPENSSLDIR: 
001db530: 2243 3a2f 5072 6f67 7261 6d20 4669 6c65  "C:/Program File
001db540: 7320 2878 3836 292f 436f 6d6d 6f6e 2046  s (x86)/Common F
001db550: 696c 6573 2f53 534c 2200 0000 454e 4749  iles/SSL"...ENGI
001db560: 4e45 5344 4952 3a20 2243 3a2f 5072 6f67  NESDIR: "C:/Prog
001db570: 7261 6d20 4669 6c65 7320 2878 3836 292f  ram Files (x86)/
001db580: 4f70 656e 5353 4c2f 6c69 622f 656e 6769  OpenSSL/lib/engi
001db590: 6e65 732d 315f 3122 0000 0000 ceb4 5d6b  nes-1_1"......]k

What's wrong with those paths? They should not be user-writable. I mean that's actually part of the OpenSSL fix for that CVE. If that's wrong it seems to me a bug against OpenSSL should get filed.

If you read the wiki above, you would know that you should use a "rule of thumb" and set --prefix/--openssldir properly. But assuming that the Tor Browser's directory is still user-writable in most installations :(, what paths should be used as safe? C:\Windows (or even %WINDIR%, if supported?) or some path in it? What is the consensus here?

Yes, the Tor Browser directory user-writable is not the issue here, though. It's that OPENSSLDIR/ENGINEDIR were user-writable. Are you claiming C:\Program Files and C:\Program Files (x86) are user-writable? If not, then why is the issue not fixed?

comment:8 Changed 3 weeks ago by gk

I mean I guess we could think about fixing this the way the curl folks did it: https://hackerone.com/reports/608577 but arguable this should happen in OpenSSL itself if needed.

comment:9 Changed 3 weeks ago by cypherpunks

What's wrong with those paths?

They are hard-coded. That's a main bug and vulnerability, in general.
Also they are not allowed for your app-local OpenSSL.

They should not be user-writable. I mean that's actually part of the OpenSSL fix for that CVE. If that's wrong it seems to me a bug against OpenSSL should get filed.

Yes.

Are you claiming C:\Program Files and C:\Program Files (x86) are user-writable?

C:\Program Files (x86) doesn't exist in 32-bit Windows.

comment:10 in reply to:  9 Changed 3 weeks ago by gk

Replying to cypherpunks:

What's wrong with those paths?

They are hard-coded. That's a main bug and vulnerability, in general.
Also they are not allowed for your app-local OpenSSL.

Where is the vulnerability here and why does it matter whether those paths are allowed for the app- local OpenSSL?

They should not be user-writable. I mean that's actually part of the OpenSSL fix for that CVE. If that's wrong it seems to me a bug against OpenSSL should get filed.

Yes.

Are you claiming C:\Program Files and C:\Program Files (x86) are user-writable?

C:\Program Files (x86) doesn't exist in 32-bit Windows.

Yeah, I just found https://github.com/openssl/openssl/pull/9400#issuecomment-517784572.

comment:11 Changed 3 weeks ago by cypherpunks

Doh, looks like you see Windows for the first time :(
What do you say when you see D:\Program Files?
Also why do you want conflicts between app-local and system-wide OpenSSL?
C:\Program Files (x86) is a WOW64 path.

comment:12 in reply to:  11 Changed 3 weeks ago by gk

Replying to cypherpunks:

Doh, looks like you see Windows for the first time :(

Actually, I do not, believe me.

What do you say when you see D:\Program Files?

I was not really talking about that. I was curious why hardcoding *any* path, like C:\Program Files on a Windows 64bit system, is a vulnerability and what would it be in that case? That's how I read your comment at least.

Also why do you want conflicts between app-local and system-wide OpenSSL?

Actually, what I want is to resolve this bug. So far, I assumed the path the OpenSSL project chose would be a good one which is why we followed it. But we might have been wrong here.

comment:13 Changed 3 weeks ago by cypherpunks

Doh, looks like you see Windows for the first time :(

Actually, I do not, believe me.

"Trust Me, I'm an Engineer" :) I know you do not, I say how it looks like. And your further questions just increase that feeling.

What do you say when you see D:\Program Files?

I was not really talking about that.

About what? D:\Program Files instead of C:\Program Files on a user's machine, and the hole is still there.

I was curious why hardcoding *any* path, like C:\Program Files on a Windows 64bit system, is a vulnerability and what would it be in that case? That's how I read your comment at least.

Hardcoding paths is a bad security practice (and not only security). Is this new for you?
Relocatable toolchain is still a miracle in a Linux world, right? On Windows, developers use environmental variables, e.g. https://www.quora.com/What-is-the-difference-between-windir-and-systemroot

comment:14 in reply to:  13 Changed 2 weeks ago by gk

Replying to cypherpunks:

Doh, looks like you see Windows for the first time :(

Actually, I do not, believe me.

"Trust Me, I'm an Engineer" :) I know you do not, I say how it looks like. And your further questions just increase that feeling.

There is no need to drag this down onto a personal level and/or starting ad hominem arguments. I told you that on different occasions in different tickets. Please stop.

What do you say when you see D:\Program Files?

I was not really talking about that.

About what? D:\Program Files instead of C:\Program Files on a user's machine, and the hole is still there.

I was curious why hardcoding *any* path, like C:\Program Files on a Windows 64bit system, is a vulnerability and what would it be in that case? That's how I read your comment at least.

Hardcoding paths is a bad security practice (and not only security). Is this new for you?

So, how are we supposed to fix this bug without introducing new vulnerabilities in your opinion? Hardcoding any path (like suggested with C:\Windows or a path below it in comment:6) like e.g. the curl devs did does not do the trick according to your line of reasoning.

comment:15 Changed 2 weeks ago by cypherpunks

There is no need to drag this down onto a personal level and/or starting ad hominem arguments. I told you that on different occasions in different tickets. Please stop.

Everything is personal in Universe. So, that is my personal amazement when no good explanation can be found. Maybe, you can explain. However, I'm still finding out what I should stop. It looks like we speak the same language, but different meanings. Relationships are far more complex than programming...

So, how are we supposed to fix this bug without introducing new vulnerabilities in your opinion?

Hey, I just read Trac from time to time :) Also expected to see Richard's suggestions here.

Hardcoding any path (like suggested with C:\Windows or a path below it in comment:6) like e.g. the curl devs did does not do the trick according to your line of reasoning.

How to teach OpenSSL to dance? Make it compatible with app-local installation, no?
For Tor Browser, the best option is to disable everything related to those paths as it doesn't use them. But you can change them to C:\Windows\Tor Browser as a so-so workaround.

comment:16 in reply to:  15 ; Changed 2 weeks ago by boklm

Replying to cypherpunks:

Hardcoding any path (like suggested with C:\Windows or a path below it in comment:6) like e.g. the curl devs did does not do the trick according to your line of reasoning.

How to teach OpenSSL to dance? Make it compatible with app-local installation, no?
For Tor Browser, the best option is to disable everything related to those paths as it doesn't use them. But you can change them to C:\Windows\Tor Browser as a so-so workaround.

Reading https://daniel.haxx.se/blog/2019/06/24/openssl-engine-code-injection-in-curl/ it seems that the issue can happen when a program loads the openssl configuration file from the default path, which is done with the openssl function CONF_modules_load_file. However we don't call this function in tor, so it doesn't look like we are vulnerable to this issue.

comment:17 in reply to:  16 Changed 2 weeks ago by gk

Resolution: invalid
Status: needs_informationclosed

Replying to boklm:

Replying to cypherpunks:

Hardcoding any path (like suggested with C:\Windows or a path below it in comment:6) like e.g. the curl devs did does not do the trick according to your line of reasoning.

How to teach OpenSSL to dance? Make it compatible with app-local installation, no?
For Tor Browser, the best option is to disable everything related to those paths as it doesn't use them. But you can change them to C:\Windows\Tor Browser as a so-so workaround.

Reading https://daniel.haxx.se/blog/2019/06/24/openssl-engine-code-injection-in-curl/ it seems that the issue can happen when a program loads the openssl configuration file from the default path, which is done with the openssl function CONF_modules_load_file. However we don't call this function in tor, so it doesn't look like we are vulnerable to this issue.

Nice find! So, I think we are actually done here.

comment:18 Changed 2 weeks ago by cypherpunks

So, the cross-compilation guidelines and best security practices were ignored. OK.

Note: See TracTickets for help on using tickets.