sys/shell: new module shell_lock by HendrikVE · Pull Request #13082 · RIOT-OS/RIOT

HendrikVE · 2020-01-10T14:23:58Z

Contribution description

This PR adds a locking mechanism to the shell, implemented by the new shell_lock module. It's not meant as a super secure system protection, it should rather be thought of as a small simple protection for demo environment purposes. For example, I recently raised a PR (#12012) for stdio over Blueooth. I might want to show a demo and control my project via bluetooth and I don't want other people to be able to connect to my device and use the shell. Using this new module you have to type in a password first. The mentioned bluetooth service only allows a single connection per device, that is an assumption that needs to be maintained, because there is only a single shell instance per device. A second user would get access to the previous unlocked shell by another user otherwise. Furthermore it's not the responsibility of this module to provide a secure channel. This needs to be done by the used communication channel, e.g. nimble in the mentioned case.
All of this doesn't mean it couldn't extended to be more secure for other purposes though.

The module shell_lock_auto_locking extends the lock mechanism by auto locking the "session" after a given timeout, which might be also interesting. Otherwise the user has to lock the shell manually by calling the added lock-command within the shell.

Testing procedure

Simply run the default application on a board. A test configuration is set in the Makefile.

fjmolinas · 2020-06-18T07:48:26Z

What PR is this waiting for @HendrikVE ?

HendrikVE · 2020-06-18T16:27:49Z

Basically, it is/was waiting for the shell refactoring PRs. Its primary use case is given once #12012 is merged. Technically I could probably do a rebase by now. I will try to do that on the weekend, but I'm not sure about merging this before the use case from #12012 is given?

HendrikVE · 2020-06-20T21:38:09Z

Rebased and squashed! No dependency on any other PR left.

miri64 · 2020-06-25T13:47:02Z

How is this related to #6893 and #12191?

miri64 · 2020-06-25T13:47:50Z

(or asking differently can #12191 be closed as this PR supersedes this one?)

HendrikVE · 2020-06-25T16:16:25Z

#6893 is a whole security layer with different user permissions, so it's quite different from what I did.

This PR is basically the idea of #12191, but without any crypto. I guess it can be closed. If we want crypto some day, we could add it to this module.

benpicco · 2022-06-01T22:52:53Z

Needs a rebase, but I think this can still be useful.

HendrikVE · 2022-06-03T16:59:51Z

Rebased to master and cleaned the PR a little. The module now uses the SHELL_COMMAND macro to put its command into shell_commands_xfa.

benpicco

I've added some commits to fix locking with the telnet server.
While this is certainly not secure, it is a cleaner workaround to the GNRC TCP issue (not generating -ECONNABORTED) than #17897.

sys/shell/shell.c

benpicco · 2022-06-05T09:38:19Z

sys/shell/shell.c

+extern void shell_lock_checkpoint(char *line_buf, int len);
+extern bool shell_lock_is_locked(void);


If you are doing that you might as well

Suggested change

extern void shell_lock_checkpoint(char *line_buf, int len);

extern bool shell_lock_is_locked(void);

#if IS_USED(MODULE_SHELL_LOCK)

extern void shell_lock_checkpoint(char *line_buf, int len);

extern bool shell_lock_is_locked(void);

#else

static inline void shell_lock_checkpoint(char *line_buf, int len)

{

(void)line_buf;

(void)len;

}

static inline bool shell_lock_is_locked(void)

{

return false;

}

#endif

and avoid cluttering the code further down with if (IS_USED(…)) blocks

Mh I don't see the benefit of doing this. At the moment it is in line with MODULE_SHELL_HOOKS and MODULE_SHELL_COMMANDS. Since these are just forward declarations we don't need empty definitions in case the module is not used at all.

sys/shell_lock/shell_lock.c

maribu · 2022-06-08T08:29:15Z

This needs a rebase now, sorry.

tests/shell_lock/tests/01-run.py

Module to lock the running shell with a password. Shell is proceeded only when the valid password was entered by the user. After 3 failed attempts, the input is blocked for a few seconds to slow down brute force attacks. Does not make use of any cryptographic features yet.

Module to lock the shell after a given timeout of time x. When the shell did not receive any input within time x, then the shell is locked automatically.

HendrikVE · 2022-06-09T13:13:01Z

Thanks for the review!

aabadie added the Area: sys Area: System label Jan 16, 2020

HendrikVE force-pushed the password_protected_shell branch 4 times, most recently from e1f92a6 to c54ebe7 Compare January 20, 2020 15:34

HendrikVE added the Type: new feature The issue requests / The PR implemements a new feature for RIOT label Feb 7, 2020

HendrikVE added the State: waiting for other PR State: The PR requires another PR to be merged first label Mar 5, 2020

HendrikVE force-pushed the password_protected_shell branch 2 times, most recently from 97b06af to d8f18ed Compare June 20, 2020 21:32

HendrikVE removed the State: waiting for other PR State: The PR requires another PR to be merged first label Jun 20, 2020

HendrikVE added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 20, 2020

HendrikVE force-pushed the password_protected_shell branch 3 times, most recently from b59518e to b8145b9 Compare June 24, 2020 12:58

fjmolinas removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 24, 2020

miri64 mentioned this pull request Jun 25, 2020

examples/default: PoC user/password login for the terminal #12191

Closed

HendrikVE force-pushed the password_protected_shell branch from b8145b9 to 2748881 Compare June 26, 2020 13:12

HendrikVE force-pushed the password_protected_shell branch from 2748881 to 9472fd4 Compare July 7, 2020 17:39

HendrikVE added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 7, 2020

HendrikVE force-pushed the password_protected_shell branch 3 times, most recently from 2819803 to c52a1e6 Compare July 7, 2020 20:47

HendrikVE force-pushed the password_protected_shell branch 2 times, most recently from 09348de to 21f9464 Compare November 6, 2020 18:15

HendrikVE force-pushed the password_protected_shell branch 2 times, most recently from ea93dfc to 7efd27a Compare March 13, 2022 22:57

HendrikVE force-pushed the password_protected_shell branch from 7efd27a to fddb08e Compare June 3, 2022 16:57

HendrikVE requested review from PeterKietzmann and haukepetersen as code owners June 5, 2022 09:50

github-actions bot added Area: examples Area: Example Applications Area: network Area: Networking labels Jun 5, 2022

benpicco reviewed Jun 5, 2022

View reviewed changes

HendrikVE force-pushed the password_protected_shell branch from 9eef476 to 4470b68 Compare June 7, 2022 13:27

benpicco approved these changes Jun 7, 2022

View reviewed changes

benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 8, 2022

maribu removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 8, 2022

maribu reviewed Jun 8, 2022

View reviewed changes

tests/shell_lock/tests/01-run.py Show resolved Hide resolved

HendrikVE and others added 6 commits June 8, 2022 12:53

sys/shell: add pseudomodule shell_lock_auto_locking

ccc7951

Module to lock the shell after a given timeout of time x. When the shell did not receive any input within time x, then the shell is locked automatically.

tests/shell_lock: add test

2f91557

sys/net/telnet: add telnet_server_disconnect()

7029572

sys/shell_lock: add telnet support

d5d08e1

examples/telnet_server: fix build with shell_lock

80b7b79

HendrikVE force-pushed the password_protected_shell branch from 4470b68 to 80b7b79 Compare June 8, 2022 11:02

HendrikVE added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 8, 2022

benpicco merged commit 8e1f908 into RIOT-OS:master Jun 9, 2022

HendrikVE deleted the password_protected_shell branch June 9, 2022 13:13

benpicco mentioned this pull request Jul 13, 2022

sys/net/telnet: automatically disconnect after period of inactivity #17897

Closed

chrysn added this to the Release 2022.07 milestone Aug 25, 2022

		extern void shell_lock_checkpoint(char *line_buf, int len);
		extern bool shell_lock_is_locked(void);

-extern void shell_lock_checkpoint(char *line_buf, int len);
-extern bool shell_lock_is_locked(void);
+#if IS_USED(MODULE_SHELL_LOCK)
+extern void shell_lock_checkpoint(char *line_buf, int len);
+extern bool shell_lock_is_locked(void);
+#else
+static inline void shell_lock_checkpoint(char *line_buf, int len)
+{
+    (void)line_buf;
+    (void)len;
+}
+static inline bool shell_lock_is_locked(void)
+{
+    return false;
+}
+#endif

Conversation

HendrikVE commented Jan 10, 2020

Contribution description

Testing procedure

Uh oh!

fjmolinas commented Jun 18, 2020

Uh oh!

HendrikVE commented Jun 18, 2020

Uh oh!

HendrikVE commented Jun 20, 2020

Uh oh!

miri64 commented Jun 25, 2020

Uh oh!

miri64 commented Jun 25, 2020

Uh oh!

HendrikVE commented Jun 25, 2020

Uh oh!

benpicco commented Jun 1, 2022

Uh oh!

HendrikVE commented Jun 3, 2022

Uh oh!

benpicco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

benpicco Jun 5, 2022

Choose a reason for hiding this comment

Uh oh!

HendrikVE Jun 7, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maribu commented Jun 8, 2022

Uh oh!

Uh oh!

HendrikVE commented Jun 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants