~sircmpwn/aerc

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
3 2

[PATCH] show perms error only if config contains secrets

Details
Message ID
<20210408175946.1015840-1-bruno@belanyi.fr>
DKIM signature
pass
Download raw message
Patch: +57 -7
---
This patch allows the user to store their configuration file with laxer
permissions if it does not contain any actual secrets (i.e: paswords or
OAuth tokens).

I'm not really familiar with Go or this codebase, so please do give me
feedback about anything that I might have missed.

 config/config.go | 64 ++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 57 insertions(+), 7 deletions(-)

diff --git a/config/config.go b/config/config.go
index 8b409fe..462b93f 100644
--- a/config/config.go
+++ b/config/config.go
@@ -162,7 +162,9 @@ func mapName(raw string) string {
	return string(newstr)
}

func loadAccountConfig(path string) ([]AccountConfig, error) {
// Load account configuration from the path, but do not extract pasword using
// provided commands
func loadAccountConfigPure(path string) ([]AccountConfig, error) {
	file, err := ini.Load(path)
	if err != nil {
		// No config triggers account configuration wizard
@@ -220,21 +222,34 @@ func loadAccountConfig(path string) ([]AccountConfig, error) {
			return nil, fmt.Errorf("Expected from for account %s", _sec)
		}

		accounts = append(accounts, account)
	}
	return accounts, nil
}

func loadAccountConfig(path string) ([]AccountConfig, error) {
	accounts, err := loadAccountConfigPure(path)
	if err != nil {
		return nil, err
	}

	var parsedAccounts []AccountConfig
	for _, account := range accounts {
		source, err := parseCredential(account.Source, account.SourceCredCmd)
		if err != nil {
			return nil, fmt.Errorf("Invalid source credentials for %s: %s", _sec, err)
			return nil, fmt.Errorf("Invalid source credentials for %s: %s", account.Name, err)
		}
		account.Source = source

		outgoing, err := parseCredential(account.Outgoing, account.OutgoingCredCmd)
		if err != nil {
			return nil, fmt.Errorf("Invalid outgoing credentials for %s: %s", _sec, err)
			return nil, fmt.Errorf("Invalid outgoing credentials for %s: %s", account.Name, err)
		}
		account.Outgoing = outgoing

		accounts = append(accounts, account)
		parsedAccounts = append(parsedAccounts, account)
	}
	return accounts, nil

	return parsedAccounts, nil
}

func parseCredential(cred, command string) (string, error) {
@@ -628,6 +643,41 @@ func LoadConfigFromFile(root *string, sharedir string) (*AercConfig, error) {
	return config, nil
}

func hasSecrets(filename string) bool {
	accounts, err := loadAccountConfigPure(filename)
	if err != nil {
		// Conservatively assume that there is a secret on errors
		return true
	}

	for _, account := range accounts {
		sourceWithPass, err := parseCredential(account.Source, account.SourceCredCmd)
		if err != nil {
			return true
		}
		if account.Source == sourceWithPass {
			return true
		}
		if strings.Contains(account.Source, "+oauthbearer://") {
			return true
		}

		outgoingWithPass, err := parseCredential(account.Outgoing, account.OutgoingCredCmd)
		if err != nil {
			return true
		}
		if account.Outgoing == outgoingWithPass {
			return true
		}
		if strings.Contains(account.Outgoing, "+oauthbearer://") {
			return true
		}
	}

	// No secrets have been identified
	return false
}

// checkConfigPerms checks for too open permissions
// printing the fix on stdout and returning an error
func checkConfigPerms(filename string) error {
@@ -637,7 +687,7 @@ func checkConfigPerms(filename string) error {
	}
	perms := info.Mode().Perm()
	// group or others have read access
	if perms&044 != 0 {
	if perms&044 != 0 && hasSecrets(filename) {
		fmt.Fprintf(os.Stderr, "The file %v has too open permissions.\n", filename)
		fmt.Fprintln(os.Stderr, "This is a security issue (it contains passwords).")
		fmt.Fprintf(os.Stderr, "To fix it, run `chmod 600 %v`\n", filename)
-- 
2.31.1
Details
Message ID
<20210412170832.ar6dej55fcdmbnac@feather.localdomain>
In-Reply-To
<20210408175946.1015840-1-bruno@belanyi.fr> (view parent)
DKIM signature
pass
Download raw message
Hi,
Thanks for the patch.

On Thu, Apr 08, 2021 at 07:59:47PM +0200, Bruno BELANYI wrote:
> This patch allows the user to store their configuration file with laxer
> permissions if it does not contain any actual secrets (i.e: paswords or
> OAuth tokens).

Why do you need this in the first place?
Your user has read access, why do other users need to read the config?

> I'm not really familiar with Go or this codebase, so please do give me
> feedback about anything that I might have missed.

> +func hasSecrets(filename string) bool {
> +	accounts, err := loadAccountConfigPure(filename)
> +	if err != nil {
> +		// Conservatively assume that there is a secret on errors
> +		return true
> +	}
> +
> +	for _, account := range accounts {
> +		sourceWithPass, err := parseCredential(account.Source, account.SourceCredCmd)
> +		if err != nil {
> +			return true
> +		}
> +		if account.Source == sourceWithPass {
> +			return true
> +		}
> +		if strings.Contains(account.Source, "+oauthbearer://") {
> +			return true
> +		}

I don't think this is the right approach, this is way to brittle.
The code that actually adds new auth schemes doesn't life in this function and
chances are this gets missed once a new scheme is introduced.
Meaning we now have a security problem waiting to happen.

> +		outgoingWithPass, err := parseCredential(account.Outgoing, account.OutgoingCredCmd)
> +		if err != nil {
> +			return true
> +		}
> +		if account.Outgoing == outgoingWithPass {
> +			return true
> +		}
> +		if strings.Contains(account.Outgoing, "+oauthbearer://") {
> +			return true
> +		}
> +	}
> +
> +	// No secrets have been identified
> +	return false
> +}
> +
>  // checkConfigPerms checks for too open permissions
>  // printing the fix on stdout and returning an error
>  func checkConfigPerms(filename string) error {
> @@ -637,7 +687,7 @@ func checkConfigPerms(filename string) error {
>  	}
>  	perms := info.Mode().Perm()
>  	// group or others have read access
> -	if perms&044 != 0 {
> +	if perms&044 != 0 && hasSecrets(filename) {
>  		fmt.Fprintf(os.Stderr, "The file %v has too open permissions.\n", filename)
>  		fmt.Fprintln(os.Stderr, "This is a security issue (it contains passwords).")
>  		fmt.Fprintf(os.Stderr, "To fix it, run `chmod 600 %v`\n", filename)
> -- 
> 2.31.1
> 

Kind regards,
Reto
Details
Message ID
<40e59e08e08936ec9737bd16c9f96e70@belanyi.fr>
In-Reply-To
<20210412170832.ar6dej55fcdmbnac@feather.localdomain> (view parent)
DKIM signature
pass
Download raw message
Hello,

On Mon, 12 Apr 2021 19:08:32 +0200
Reto <reto@labrat.space> wrote:
> Why do you need this in the first place?
> Your user has read access, why do other users need to read the config?

There have been multiple issues relating to this check: [1] and [2].

> I don't think this is the right approach, this is way to brittle.
> The code that actually adds new auth schemes doesn't life in this
> function and chances are this gets missed once a new scheme is
> introduced. Meaning we now have a security problem waiting to happen.

I only added the path about auth schemes because it was brought up
somewhere (I cannot find the link anymore).

What if instead we only deactivate the warning when only `*-cred-cmd`
options are used? 

[1] https://lists.sr.ht/~sircmpwn/aerc/%3CC1HLQYU8QSUI.3GYFQ9WJOH7H6%40ginger%3E
[2] https://lists.sr.ht/~sircmpwn/aerc/%3CC0KLIH5UWE2G.LPUOBCRVFD5S%40wrt%3E

Respectfully,

-- 
Bruno BELANYI
Details
Message ID
<20210412201828.mujuyufv6okdkvxj@feather.localdomain>
In-Reply-To
<40e59e08e08936ec9737bd16c9f96e70@belanyi.fr> (view parent)
DKIM signature
pass
Download raw message
> What if instead we only deactivate the warning when only `*-cred-cmd`
> options are used?

Hm, that might work.
But that means bailing out as soon as even one account doesn't have it.

Cheers,
Reto
Reply to thread Export thread (mbox)