Skip to content

Screen change script argument#249

Merged
AdrianKoshka merged 3 commits into
debauchee:masterfrom
vorph1:screen-change-script
Feb 10, 2019
Merged

Screen change script argument#249
AdrianKoshka merged 3 commits into
debauchee:masterfrom
vorph1:screen-change-script

Conversation

@vorph1
Copy link
Copy Markdown

@vorph1 vorph1 commented Feb 5, 2019

Added a new server cli argument --screen-change-script that is invoked with the screen name as the first parameter.

My main use case for barrier is for VMs with GPU passthrough so I use this to switch my monitor input ie.

#!/bin/bash
case "$1" in
    "WIN10")
        notify-send "Changing to guest output"
        sudo ddcutil -d 1 setvcp 0x60 0x01
        ;;
    "desktop")
        notify-send "Changing to host output"
        sudo ddcutil -d 1 setvcp 0x60 0x03
        ;;
esac

This is for linux server only.

@AdrianKoshka
Copy link
Copy Markdown

So, if I understand, this allows barrier to execute arbitrary code upon the condition of screen change?

@AdrianKoshka AdrianKoshka added the enhancement New feature or request label Feb 5, 2019
@vorph1
Copy link
Copy Markdown
Author

vorph1 commented Feb 5, 2019

Yup, that's correct

Comment thread src/lib/barrier/ServerApp.cpp Outdated
cmd += info->m_screen;
cmd += " &";

system(cmd.c_str());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is C++ code, use std::system.

@AdrianKoshka
Copy link
Copy Markdown

I don't know how I feel about allowing barrier to execute arbitrary code. What is your opinion @p12tic ?

@vorph1
Copy link
Copy Markdown
Author

vorph1 commented Feb 7, 2019

It's running a user provided script with user configured screen names so it shouldn't be a security issue if that's your concern. Also since it's only available as a command line argument it's available only to users that run barriers manually so they should know what they're doing :)

@p12tic
Copy link
Copy Markdown
Member

p12tic commented Feb 7, 2019

I agree with @vorph1. If attacker has access to command line arguments he already can do whatever he wants. Looking from this side, there's one issue though. info->m_screen as an argument to std::system is not escaped. Something like QProcess would be a better idea instead of std::system.

@AdrianKoshka
Copy link
Copy Markdown

It's not like barrier runs in a privileged mode anyway

@vorph1
Copy link
Copy Markdown
Author

vorph1 commented Feb 7, 2019

This would add a Qt dependency for the sever app.

@jwestfall69
Copy link
Copy Markdown

You could use execl() instead of system().

@p12tic
Copy link
Copy Markdown
Member

p12tic commented Feb 9, 2019

Hm, execl() is linux-only. Maybe something from https://www.boost.org/doc/libs/1_64_0/doc/html/process.html would be a better cross-platform option.

@jwestfall69
Copy link
Copy Markdown

execl() is part of POSIX.

@p12tic
Copy link
Copy Markdown
Member

p12tic commented Feb 9, 2019

OK, I missed the part that this is linux-only. Looks good then.

@AdrianKoshka
Copy link
Copy Markdown

Ready to be merged then?

@p12tic
Copy link
Copy Markdown
Member

p12tic commented Feb 10, 2019

Yes, let's do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants