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

[SSHD-1324] Rooted file system can leak informations #362

Merged
merged 1 commit into from May 9, 2023

Conversation

gnodet
Copy link
Contributor

@gnodet gnodet commented Apr 18, 2023

  • More unix like
  • Symlinks improvements

Copy link
Contributor

@lgoldstein lgoldstein left a comment

Choose a reason for hiding this comment

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

I have not looked yet in depth - at cursory glance, there are some visibility issues. Basically, IMO in any open-source project the overwhelming if not all of the methods/fields/definitions should be public or protected in order to provide users the maximum possible flexibility to override and/or use the code.

@tomaswolf
Copy link
Member

It would be extremely helpful to have meaningful commit messages. Just "More unix like" is not helpful. If someone looks at this code a few months or years from now he or she will have no idea what this is about from the git history. There's not even any link to any issue, PR, or mailing list message. In this case I do not expect the commit message to give a full explanation, but brief descriptions should be possible.

At the very least there must be a meaningful summary line so that git log --oneline gives a result that correctly summarizes the scope and topic.

Absent a JIRA or Github issue, a link back to this PR might also helpful.

Something like

RootedFileSystem: more Unix-like path handling

Clean up and fix path checks to be more like in Unix.
Add unit tests for Unix, OSX, and Windows via JimFs.
See also [1].

[1] https://github.com/apache/mina-sshd/pull/362

should be the minimum for the first commit. And likewise for the second commit.


private Path getTargetFolderOnHostFs(Path targetFolder) {
return this.hostFilesystem.getSeparator().equals("\\")
? this.hostFilesystem.getPath("C:", targetFolder.toString())
Copy link
Member

Choose a reason for hiding this comment

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

This looks suspicious. In CI, we're on drive D:.

Copy link
Contributor

Choose a reason for hiding this comment

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

I second that - someone else might run on drive X: or Z: (or any other letter of the alphabet). The code should auto-detect if running on Windows and then also auto detect the drive letter.

@pyckle
Copy link

pyckle commented Apr 23, 2023

I made the suggested changes here: https://github.com/gnodet/mina-sshd/pull/2/files

@gnodet gnodet changed the title Fix rooted filesystem [SSHD-1324] Rooted file system can leak informations May 2, 2023
@gnodet
Copy link
Contributor Author

gnodet commented May 2, 2023

@tomaswolf I plan to squash the commits into a single commit

@tomaswolf
Copy link
Member

@gnodet: seems this needs more work. I suggest we publish a 2.10.0 without this change, and do a 2.10.1 once this is ready. I'd really like to have 2.10.0 in Eclipse 2023-06, but to make that work, I'll need a release next week. Otherwise it'll be too late in the Eclipse release process.

@gnodet gnodet force-pushed the root-fs branch 4 times, most recently from 3d27135 to 2b13c8a Compare May 9, 2023 15:38
@lgoldstein
Copy link
Contributor

I suggest we publish a 2.10.0 without this change, and do a 2.10.1 once this is ready.

I second that...

@gnodet gnodet merged commit a61e930 into apache:master May 9, 2023
8 checks passed
@tomaswolf tomaswolf added this to the 2.10 milestone May 17, 2023
@07070529
Copy link

07070529 commented Aug 3, 2023

@gnodet Hi, we see from the NVD that this issue is related to vulnerability CVE-2023-35887 and affect Apache MINA(https://nvd.nist.gov/vuln/detail/CVE-2023-35887).
image

Then in the content of the reference link (https://lists.apache.org/thread/b9qgtqvhnvgfpn0w1gz918p21p53tqk2), we see:
This issue affects Apache MINA: from 1.0 before 2.10.
image

Sorry, we're a little confused:

  1. The affected software scope is different with the title (Affected versions: Apache MINA SSHD 1.0 before 2.10).
  2. We looked for a lot of information, including the Apache MINA community(https://mina.apache.org/mina-project, http://issues.apache.org/jira/browse/DIRMINA), but we didn't see any discussion about whether this issue affected the Apache MINA.

We'd greatly appreciate it if you could give us some advice on whether the CVE-2023-35887 vulnerability affects Apache MINA 2.1.X and 2.2.X?

@gnodet
Copy link
Contributor Author

gnodet commented Aug 3, 2023

We'd greatly appreciate it if you could give us some advice on whether the CVE-2023-35887 vulnerability affects Apache MINA 2.1.X and 2.2.X?

The issue affects the Apache Mina SSHD project, not the Apache Mina library.

@07070529
Copy link

07070529 commented Aug 4, 2023

Thanks a lot for your reply, and we appreciate your prompt attention to this issue.

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