Skip to content
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

.RDP Password Attribute #3756

Merged
merged 2 commits into from
Feb 17, 2017
Merged

.RDP Password Attribute #3756

merged 2 commits into from
Feb 17, 2017

Conversation

bigpjo
Copy link
Contributor

@bigpjo bigpjo commented Feb 10, 2017

Allow password to be stored in .RDP file, parsed and settings
updated, this will allow for dynamic .RDP files to be created with
complete login credentials, using this method the username, server and
password will no longer be visible within process lists.

Also fixed bug with username being
set to null by command line processor if the command line parameter not specified but the value set from the .RDP files.

Usage: create .RDP file, add line

password:s:XXXXXX where XXXXXX is the password

Allow password to be stored in .RDP file and parsed and settings
updated, this will allow for dynamic .RDP files to be created with
complete login credentials, using this method the username, server and
password will no longer be visible within process lists.

Also fixed issue of username and domain being read from .RDP files and
set to null by command line processor.
@freerdp-bot
Copy link

Can one of the admins verify this patch?

@akallabeth
Copy link
Member

@freerdp-bot test

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/1780/

@akallabeth
Copy link
Member

@freerdp-bot test please

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/1782/

free(settings->Username);

if (!settings->Domain && user)
if (!settings->Username && user)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is no good idea. If I want to override the username provided via RDP file this check will prevent it. A simple if (user) should do the trick.

@akallabeth
Copy link
Member

@bigpjo Sorry for the change of mind, just found this issue when retesting your pull.

@bmiklautz
Copy link
Member

As a remark: Although this is a possible solution we haven't added "password" support in .rdp files yet because you then possibly have files lying around with plain passwords. So if you don't wan't to show the user/server/domain/password in the processes list you probably also don't want to have the password in files. I think currently the best solution is to create a wrapper script and use /from-stdin.

Copy link
Member

@bmiklautz bmiklautz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except for @akallabeth 's remarks I looks good to me. Thanks.

@bigpjo
Copy link
Contributor Author

bigpjo commented Feb 15, 2017

I appreciate that storing plain text passwords in the file are not ideal and this is not a suggestion/change I would recommend without a specific goal in mind, there are a number of other open source projects dependent on starting FreeRDP from a command line, it is these projects that would benefit from this change as it would be possible to create .RDP files with a password, start the process and delete the file after. Not ideal but a solution.

If it would be of benefit, I could also add another command line parameter to delete the .RDP file once the process is started to ensure that there are not 100's of .RDP files with passwords left around but this would still be user choice.

In .RDP files there is a hashed password field password 51, the issue with this is that you need a windows API call for the RDP client to decrypt this and send it to establish the report session, this will not work I believe in a linux/mac/unix env making this type of change non portable.

Any comments welcomed, in the grand schema of things, this pull request is not a major contribution to an excellent project

@hardening
Copy link
Contributor

My 2 euros cents: previous version of mstsc on mac used to support the password in clear.

@akallabeth
Copy link
Member

@bigpjo Agree with the necessity of storing the password in some cases.
As for other projects, as @bmiklautz already pointed out providing the password via stdin is probably a better solution (but also far from perfect)
The hashed password option is, as you correctly asserted, not an option without introducing some kind of password manager dependency

That aside, if you fix the check for allowing command line to override RDP username you got my vote ;)

Change to allow username command line parameter to overwrite the .RDP
username
@akallabeth
Copy link
Member

@freerdp-bot test

@akallabeth
Copy link
Member

@freerdp-bot test, come on

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/1836/

@akallabeth akallabeth merged commit 4065581 into FreeRDP:master Feb 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants