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

Rework host authentication for SFTP storage backends #639

Open
orangelynx opened this issue Nov 17, 2022 · 0 comments
Open

Rework host authentication for SFTP storage backends #639

orangelynx opened this issue Nov 17, 2022 · 0 comments

Comments

@orangelynx
Copy link

orangelynx commented Nov 17, 2022

func checkHostKey(hostname string, remote net.Addr, key ssh.PublicKey) error {
if preferencePath == "" {
return fmt.Errorf("Can't verify SSH host since the preference path is not set")
}
hostFile := path.Join(preferencePath, "known_hosts")
file, err := os.OpenFile(hostFile, os.O_RDWR|os.O_CREATE, 0600)
if err != nil {
return err
}
defer file.Close()
content, err := ioutil.ReadAll(file)
if err != nil {
return err
}
lineRegex := regexp.MustCompile(`^([^\s]+)\s+(.+)`)
keyString := string(ssh.MarshalAuthorizedKey(key))
keyString = strings.Replace(keyString, "\n", "", -1)
remoteAddress := remote.String()
if strings.HasSuffix(remoteAddress, ":22") {
remoteAddress = remoteAddress[:len(remoteAddress)-len(":22")]
}
for i, line := range strings.Split(string(content), "\n") {
matched := lineRegex.FindStringSubmatch(line)
if matched == nil {
continue
}
if matched[1] == remote.String() {
if keyString != matched[2] {
LOG_WARN("HOSTKEY_OLD", "The existing key for '%s' is %s (file %s, line %d)",
remote.String(), matched[2], hostFile, i)
LOG_WARN("HOSTKEY_NEW", "The new key is '%s'", keyString)
return fmt.Errorf("The host key for '%s' has changed", remote.String())
} else {
return nil
}
}
}
file.Write([]byte(remote.String() + " " + keyString + "\n"))
return nil
}

Just something I stumbled across trying to understand how duplicacy verifies the host it is connecting to.

  1. This code is not fully compatible with the standard definition of the SSH known_hosts file syntax [1]. Perhaps it's not meant to be, but then the name of the file is misleading. While the code appears to be able to account for markers, it does not seem to be able to account for the brackets around the host name, in case a non-standard port is provided. Also if the standard port :22 is used, it is not enforced that it is omitted in the config file.

  2. I'd argue that duplicacy should not automatically connect to hosts for which it has never seen a fingerprint before, without explicit user consent. Right now, a MITM could be happening during the first use of duplicacy, leading to exposed data.

  3. Maybe consider taking into account ~/.ssh/known_hosts as well in addition to .duplicacy/known_hosts?

[1] https://man.openbsd.org/sshd#SSH_KNOWN_HOSTS_FILE_FORMAT

@orangelynx orangelynx changed the title Do not assume SSH port 22? Rework host authentication for SFTP storage backends Nov 17, 2022
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

No branches or pull requests

1 participant