Simple security changes

[lllllllillllllillll]

Two simple changes to significantly increase security:

Localhost only: Don't allow any commands to be executed unless they are received from 127.0.0.1 (localhost), which would be the same machine that script-orchestra is running on. This would require you to use a secure connection, like a VPN or SSH, to remotely execute commands.

Limited input: To prevent someone from issuing a command you wouldn't want, like wiping all your data, user input should get matched to value.

ex.

  let user_input = req.body.command;
    switch (user_input) {
        case 'ls': 
                // list command
            break;
        case 'ping':
                // ping command
            break;
    }

instead of the more dangerous:

    let user_input = req.body.command;
    exec(user_input_that_could_be_anything)

[I haven't looked through everything, so I'm not sure if you filtered out user the user input a different way]

[calumk]

Localhost only: Don't allow any commands to be executed unless they are received from 127.0.0.1 (localhost), which would be the same machine that script-orchestra is running on. This would require you to use a secure connection, like a VPN or SSH, to remotely execute commands.

This would defeat the point of this package, which is specifically to ALLOW remote triggering. I could however add a flag so that it can be disabled so only local is allowed - might be useful for a small use case.

---

let user_input = req.body.command;
exec(user_input_that_could_be_anything)

[I haven't looked through everything, so I'm not sure if you filtered out user the user input a different way]

[!NOTE] The code you reference above does not exist anywhere in the package - that is not how this works, and opening an isue that states it is might put people off using this package.

The actual code is as follows.

    // find the command to run given the group_id and command_id 
    // these are properties of the objects sent on button click. The actual command is not sent from the button click, just a reference to which command to run.
    let command_to_run = commands_file.command_groups.find(cg => cg.id == group_id).commands.find(c => c.id == command_id)
    // the command to run is in the command_to_run.command property
    let command = command_to_run.command
    let allOtherArgs = command.slice(1)
    // Run the command
    command_to_run.child_process = spawn(command[0], allOtherArgs, {cwd:"commands_working_directory"});

I appreciate you trialling this package, but please actually read the code before submitting an issue on how the code works

[calumk]

I should add, that at the moment there is no error checking, no way of catching if a command id is valid etc, and limited user feedback. I am not saying the system is currently secure - I am just saying that it is not insecure in the specific way you mention!

[lllllllillllllillll]

This would defeat the point of this package, which is specifically to ALLOW remote triggering. I could however add a flag so that it can be disabled so only local is allowed - might be useful for a small use case.

I am just saying that it is not insecure in the specific way you mention!

Is your goal to allow anyone on any network to connect to your machine and run anything they want? Just wanted to suggest restricting to localhost and preventing certain commands from being executed, like "rm".

The code you reference above does not exist anywhere in the package - that is not how this works, and opening an isue that states it is might put people off using this package.

I thought labeling it example and calling the variable "user_input_that_could_be_anything" made that obvious.

[calumk]

Is your goal to allow anyone on any network to connect to your machine and run anything they want?

No

Commands are pre-specified in the commands.json file.

Only pre-specified commands can be run.

[lllllllillllllillll]

Yeah, I see how it matches the IDs with the commands in commands_file.

To re-use my ( completely made up ) example:

switch (command_from_commands_file) {
    case 'ls': 
            exec(`ls ${value from commands_file}`);
        break;
    case 'ping':
            // ping command
        break;
}

You can exponentially reduce the chances of a machine breaking, or becoming compromised, by hard-coding the commands you need to make available.

I hope that explains it better. security vs convenience. Pro: It will execute anything. Con: It will execute ANYTHING.

[calumk]

This argument is going in circles. There is no functional security difference between your case switch suggestion, and the current implementation.

The hard-coded commands completely defeats the point of the commands.json file, and the goal of this prioject.

Once the application is compiled to a binary, the only way for the user to adjust the commands is through the external commands.json file.