proxmoxblock fields can leak username/password

[FliesLikeABrick]

This behavior may be specific to Firefox, I have not investigated other browsers.

The proxmox API username/password fields are hidden in the device details view in opendcim, however some browsers appear to be auto-filling saved opendcim credentials into these fields when the form is submitted. This leads to the username and password of the user being saved in the opendcim database in plaintext.

Further, when someone else views that device details page, the username and password are sent to the browser in plaintext as they are the current values for the field.

Any user with privileges to view device details is able to see these credentials and potentially use them for malicious purposes, privilege escalation elsewhere in the environment, etc -- in particular when OpenDCIM is authenticating against corporate AD/LDAP

Perhaps these fields should have the "autocomplete"="off" attribute added, which appears to be honored by many/most/all browsers of consequence, to prevent autofill. There is no reason why OpenDCIM would want hidden fields to be autofilled by browsers in general, but specifically for this concern.

https://www.w3docs.com/snippets/html/how-to-disable-browser-autocomplete-and-autofill-on-html-form-and-input-fields.html

In our organization's environment, I have located over 1000 devices with what appear to be valid active directory credentials

I am happy to open a PR with this proposed fix

<fieldset id="proxmoxblock" class="hide">
    <legend>ProxMox Configuration</legend>
    <div class="table">
        <div>
          <div><label for="APIUsername">API Username</label></div>
          <div><input type="text" name="APIUsername" id="APIUsername" value="tderavin"></div>
        </div>
        <div>
          <div><label for="APIPassword">API Password</label></div>
          <div><input type="password" name="APIPassword" id="APIPassword" value="tderavin_user_plaintext_password"></div>
        </div>
        <div>
          <div><label for="APIPort">API Port</label></div>
          <div><input type="number" name="APIPort" id="APIPort" value="0"></div>
        </div>
        <div>
          <div><label for="ProxMoxRealm">ProxMox Realm</label></div>
          <div><input type="text" name="ProxMoxRealm" id="ProxMoxRealm" value=""></div>
        </div>
    </div>

[wilpig]

This has been discussed before and if you want to submit a pull with the tag on it i'll merge it. I tried the tag in the past and browsers just didn't honor it and I didn't care if someone was auto-filling.

https://github.com/opendcim/openDCIM/commit/6e96fd1f4ae189ed7958776ebb4b8c4c6b1d3f0e https://github.com/opendcim/openDCIM/commit/9b54a9383c05ffc2664334f759ec51ce28d61b53

I went back to the archives to find this.

That tag IS in the current code base, what version are you running? https://github.com/opendcim/openDCIM/blame/master/devices.php#L1781 https://github.com/opendcim/openDCIM/blame/master/devices.php#L2061 https://github.com/opendcim/openDCIM/blame/master/devices.php#L2065

[FliesLikeABrick]

Looks like we are on 20.2 - I will talk with our actual opendcim admin about upgrading to something more recent - it looks like 21.01 didn't have this tag, but 23.01 does.

[FliesLikeABrick]

@wilpig can you add some context about why these hidden fields exist in the page in the first place? Is it set to class="hide" when a certain feature is disabled or something; and if so would it be better to just have this part of the form not be printed/echoed into the HTML document in the first place?

[wilpig]

https://github.com/opendcim/openDCIM/blame/master/devices.php#L1393

Because UI's are dumb and if we want the blanks translated then it has to go through something like this or use another unit to translate then import. Having the content available on demand is just easier.

[wilpig]

If you aren't going to update to the latest and just add those properties to the fields you should also backport the changes to the logging class. https://github.com/opendcim/openDCIM/blob/master/classes/LogActions.class.php

[FliesLikeABrick]

Thanks, I am hoping to get our team to update to the latest since there are other changes I want to adopt (including a merged PR I submitted early this year)

[FliesLikeABrick]

We manually backported the autocomplete=off changes into our 20.0x devices.php and it did not seem to have the desired effect. We hopefully will upgrade to 23.01 next week in case we missed anything else of consequence. @wilpig if you could spare the time, can you explain a little more about the translation limitations? If 23.01 does not improve the behavior here, I would like to understand more about the issues you ran into with disabling fields elegantly, so that I might contribute a PR with alternate solutions.

[FliesLikeABrick]

Could another option be to change the form fieldset that is printed when it is intended to be hidden -- could we make those elements all become type=hidden instead of text/password? I have to imagine browsers will not autofill hidden fields. The reason they're autofilling the current class=hide fields is since those just aren't being rendered... the browser isn't smart enough to try and figure out if they're visible to determine whether to autofill

[FliesLikeABrick]

PR #1396 opened with a fix based on autocomplete="new-password" which is supported by at least Firefox, Chrome/ium, and Safari. This has the behavior that was originally desired when autocomplete="off" was used.

If this can be included in 23.02, it would be greatly appreciated and help us close out a security deficiency that is being tracked in our organization against our opendcim deployment; versus having to deploy 23.01 or 23.02 and maintain this manual patch.

[FliesLikeABrick]

New fix proposed in #1398 to set the fields to type=hidden when they are non-rendered to the user

[FliesLikeABrick]

1398 has been merged which should improve the behavior here