Skip to content

Conversation

@ceolin
Copy link
Member

@ceolin ceolin commented Jun 9, 2020

Add security information about enabling fs management for mcumgr.

Closes #22340

@ceolin ceolin added area: Security Security Security Review To be reviewed by a security expert labels Jun 9, 2020
@ceolin ceolin requested review from Laczen, d3zd3z and dleach02 June 9, 2020 16:40
@dleach02
Copy link
Member

dleach02 commented Jun 9, 2020

while this PR documents the security problems with FS, are we in general agreement that this should close #22340? Documenting the problem was only one of the potential solutions and I'm not sure there was general agreement that it would be acceptable to close the issue if we did document it?

@ceolin
Copy link
Member Author

ceolin commented Jun 9, 2020

while this PR documents the security problems with FS, are we in general agreement that this should close #22340? Documenting the problem was only one of the potential solutions and I'm not sure there was general agreement that it would be acceptable to close the issue if we did document it?

That is still an open question, I particularly don't think restrict access to specific folders is a valid fix, we would just hiding a bigger problem.
My understanding is, unless we have a valid use case this feature should never be used in production, besides that, the control access must be done in mcumanager, it access Zephyr's APIs directly, of course, we ship it so we have to address this problem.

Again, unless we have a better proposal I think that is the way to go.

Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't read well. Something like this would be more clear.

This option allows mcumgr clients to access anything in the file system, including application-stored secrets like private keys. Use of this feature in production is strongly discouraged.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, thanks !

Add a warning in fs mgmt option about security risks.

Signed-off-by: Flavio Ceolin <[email protected]>
Copy link
Contributor

Choose a reason for hiding this comment

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

newline

MCMUMGR file system management is discouraged in production, just
adding it to the recommendation list.

Signed-off-by: Flavio Ceolin <[email protected]>
@carlescufi carlescufi merged commit b429b12 into zephyrproject-rtos:master Jun 12, 2020
@zephyrbot
Copy link

The backport to v1.14-branch failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-v1.14-branch v1.14-branch
# Navigate to the new working tree
cd .worktrees/backport-v1.14-branch
# Create a new branch
git switch --create backport-26083-to-v1.14-branch
# Cherry-pick the merged commits of this pull request and resolve the conflicts
git cherry-pick b429b126099caa5e7984719aebe77583cdf25d36~2..b429b126099caa5e7984719aebe77583cdf25d36
# Push it to GitHub
git push --set-upstream origin backport-26083-to-v1.14-branch
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-v1.14-branch

Then, create a pull request where the base branch is v1.14-branch and the compare/head branch is backport-26083-to-v1.14-branch.

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

Labels

area: Kconfig area: Security Security Security Review To be reviewed by a security expert

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Security problem with settings and littlefs

8 participants