Security Issue in establishing web socket handshake

Badranh commented 3 months ago

Hello,

I was skimming through the code, to do a successful web socket handshake all you need is a valid ID and valid Protocol and after that you are able to create and craft any payloads to change the connector data.

Looking at the input from security POV: I don't think that the WS server url, connector, OCPP version are hard to obtain while OCPP already have suggested an approach which is basic authentications:

https://library.e.abb.com/public/8f07987a3a284da6bf4e4f8f53cd6502/ABB_Terra_AC_Charger_OCPP1.6_ImplementationOverview%20_v1.8_FW1.6.6.pdf

currently, basic auth is not existing in this project and I'm not sure if all chargers support it, but some for sure and I highly recommend it.

https://www.yumpu.com/en/document/read/65897073/ocpp-16-security-whitepaper-edition-2/3#google_vignette.

The scenario is that a hacker can control data of the dashboard with any form of authentication

Checklist

[x] I checked other issues already, but found no answer/solution
[x] I checked the documentation and wiki, but found no answer/solution
[x] I am running the latest version and the issue still occurs
[x] I am sure that this issue is about SteVe (and not about the charging station software or something unrelated to SteVe)

Specifications

Using docker instance.

Expected Behavior

Reject requests with no authentication headers.

Actual Behavior

Allowing all requests no authentication is required, only a valid ID.

Steps to Reproduce the Problem

Install Steve using docker or any enivorement
Create chargebox with ID 1234
Connect through websockets and send requests

goekay commented 3 months ago

Reject requests with no authentication headers.

we cannot do that since it would break existing behaviour with stations that do not work with auth.

this is the problem with this project: i am (or we are) not a CPO with a tight control of stations that i have in my system. therefore steve is not tailor-made for some specific requirement. this is a generic application that can be used by CPOs for many types of uses cases and stations. that reduces the security level to really broad common denominator.

we can implement the basic auth for ocpp 1.6 though for stations that support it to use it.

Badranh commented 3 months ago

I understand, do you think offering it as optional would also break the behaviour?

Upon creation of the charger, we could offer an option to use auth or no.

However, if this is still a big change or breaking I would advise mentioning this in the readme since not all users of steve will be aware and some may do an insecure setup.

goekay commented 3 months ago

no, it is not a breaking change.

during the ws connection initiation we can check for basic auth header being set or not.

if not set, continue as usual (current flow).
if set, validate the values against the database
- if valid: let ws connection to happen
- if invalid: reject request

Badranh commented 3 months ago

I would add during the setup of the charge point , a checkbox to select if this charger point is meant to be secured or no.

Thanks a lot, great work though!

goekay commented 3 months ago

i would suggest to modify the "add chargepoint" page to contain two new fields: WS/JSON basic auth user and basic auth password.

if both of these fields are set, then it is implied that this chargepoint wants auth. and then this checkbox is not needed.

goekay commented 3 months ago

i am closing this issue since it addresses the same topic. feel free to re-open if you believe there is a difference, or we can continue the discussion in the referenced issue.

Badranh commented 2 months ago

Hello @goekay, if it's okay and for documenting purposes I would like to publish a CVE regarding this issue.

A CVE have already been obtained but I would like to get your confirmation on making the CVE published to the records :

CVE-2024-44843

Best regards.

goekay commented 2 months ago

basic auth for ocpp-j has been added in a later edition of the spec.

steve implements the first edition of the spec. in this version, there was no talk about security using basic auth.

the fact that steve does not implement this feature has been documented numerous times in different issues. i would argue there is no vulnerability in steve regarding this, because it is not implemented. if it were implemented but wrongly, and contained leaks, bugs or if some security-relevant aspects were overlooked etc... then this would have been a CVE.

you equate something not implemented to a vulnerability that warrants a CVE?

Badranh commented 2 months ago

@goekay Thanks for the reply.

When assessing security, it’s important to know that not implementing a security measure is the same as mis-implementing it, like basic authentication, in an environment where it is expected, exposes the system to potential vulnerabilities.

Take the example not implementing input validation, where an attacker could perform SQL injection. you could choose not to implement validations ( and know it ) or you could implement it in the wrong way on both ways you are still vulnerable to SQL injections.

Those using steve may assume that it is compliant with modern security practices for OCPP 1.6, but without the proper documentation and awareness, they may overlook the associated risks and get compromised

Regardless of what led to something, the end result is that the users of steve can be compromised if they are not willing to look deeply in the code and discover that basic auth is not implemented or motivate people to use insecure connections or allowing attackers to easily find targets.

Taking from the readme file:


Charge Point Support
Electric charge points using the following OCPP versions are supported:

OCPP1.2S
OCPP1.2J
OCPP1.5S
OCPP1.5J
OCPP1.6S
OCPP1.6J

juherr commented 2 months ago

@Badranh sql injection is really a problem when it leaks data. What data are you able to leak in steve via the ocpp interface?

Many stations are not supporting "security" and won't. When you choose steve as csms, you expect that it is "not secured".

In that situation what could be the fix for your cve?

Sorry but it really sounds like a wrong positive here that will just create a wrong feeling about the project.

Badranh commented 2 months ago

@juherr as much as sql injection will leak data, but using what I mentioned you are allowing unauthorized access to control your connected chargers SteVe which is even more dangerous.

Many stations are not supporting "security" and won't.

Does it mean that we shouldn't offer "security" ? as 1.6 already suggests that it have be used.

you expect that it is "not secured".

Based on what? is there any disclaimer that warns about it? or is it because it is an open source app?

In that situation what could be the fix for your cve?

The fix is to create authorization between the charger and steve and make sure that the requests are coming from authorized entity and not any entity using basic auth or any other method you prefer.

If this project is meant not to be "secure" then this have to be mentioned somewhere so people/companies using this project must stay away from using it in prod unless secured.

There is also a recommendation for people who are offering this service for others using SteVe, but not security warning?

If you are in the EU and offer vehicle charging to other people using SteVe, keep in mind that you have to comply to the General Data Protection Regulation (GDPR) as SteVe processes charging transactions, which can be considered personal data.

I truly appreciate all the efforts that are done to create this project and I see big number of people are using or trying to use it and they deserve to be informed that if an attacker know there SteVe domain/ip address that they can be easily compromised.

We must provide a clear documentation that states the current state of the project and that it is not secure otherwise I firmly believe this is a security issue that have to be resolved.

goekay commented 2 months ago

When assessing security, it’s important to know that not implementing a security measure is the same as mis-implementing it, like basic authentication, in an environment where it is expected, exposes the system to potential vulnerabilities.

fair enough.

Take the example not implementing input validation, where an attacker could perform SQL injection. you could choose not to implement validations ( and know it ) or you could implement it in the wrong way on both ways you are still vulnerable to SQL injections.

kinda wrong example. i can name you design decisions that do not involve input validation but still do not cause sql injection. hint: prepared statements. but... i get your motivation behind the example.

Those using steve may assume that it is compliant with modern security practices for OCPP 1.6, but without the proper documentation and awareness, they may overlook the associated risks and get compromised.

the fact that steve does not support basic auth has been documented in various issues. it is not something that we are trying to hide even.

Regardless of what led to something, the end result is that the users of steve can be compromised if they are not willing to look deeply in the code and discover that basic auth is not implemented.

once again, you dont have to study the source code to come to this conclusion. we have various github issues. lets take a different perspective: a user that assumes steve is secure and wants to use a "secure" station with steve. this station would have basic auth enforced, right? right. the user will try to enter the credentials somewhere in steve, will not find it, and this is the end. he/she will immediately come to the conclusion that this is not supported by steve.

there is no magic we apply to make steve look safe and secure, whereas under the hood it is not secure. we are not. look, i want to repeat: the fact steve does not support basic auth is documented, and no one is trying to hide this. what value will a CVE bring? your arguments are shallow.

i have a feeling that you have no business with ocpp or steve. are you even using it? are you in the EV/EVSE domain? since steve reached some certain level of stars and forks, it started to get attention from some "left field" github users. i am under the assumption that you were probably scanning codebases where you were hoping to poke some holes maybe and cause a wave. maybe be an author of a CVE (that is useless), and have your 15 minutes of fame? and put this in your CV or business profile maybe?

my assumption is supported by the fact that i gave you the recipe how to implement this feature 3 weeks ago (after our nice exchange), and apparently you did not do it during this time. so, the angle you are coming from is not sincere.

i am not saying "we dont need basic auth". we need it, sure, it would improve the security. i am not trying to downplay CVEs either. we had a legit one recently. your CVE is not a legit one. but, you know... go on if you want.

Badranh commented 2 months ago

@goekay I believe you are attacking me personally while I was discussing technical stuff with all politeness with you, I am sorry that you feel attacked.

If you truly believe that this is not a problem then it is not and you are free but you have no right to do false accusations.

I already have a lot of achievements in this field and not looking for fame on the back of your app.

You can find my name listed in Meta hall of fame ( Instagram, Facebook, Whatsapp ) for 3 years, Revolut, Google, T-Mobile and many other places so SteVe is the last place I would be looking for fame.

Have a good day and I wish you the best of luck in this project

steve-community / steve