Escaping is broken

[zeripath]

Please give general description of the problem

Go-ini does not properly cope with characters that need to be escaped.

Please provide code snippets to reproduce the problem described above

• Let's look at key name escaping:

ini/file.go

Lines 332 to 339 in 3be5ad4

	switch {
	case key.isAutoIncrement:
	kname = "-"
	case strings.Contains(kname, "\"") || strings.ContainsAny(kname, f.options.KeyValueDelimiters):
	kname = "`" + kname + "`"
	case strings.Contains(kname, "`"):
	kname = `"""` + kname + `"""`
	}

Say you have a key name which contains a " and a `, lines 335 and 336 will mean that you wrap the key in `, however, leading to an unescaped component. So for example the key:

A pathological `c"as"`e

becomes:

`A pathological `c"as"`e`

Which is likely to be interpreted as:

A pathological case

Let's try a value injection say you provide a key named like this:

KEY`=FAKE_VALUE ;"`

This becomes:

`KEY`=FAKE_VALUE ;"`` = ACTUAL_VALUE

If say you simply promote the """ case higher - then you're not protecting against embedded """ strings within the key.

I presume that key names are not allowed to have newlines in them and that this is prevented elsewhere. If not then this is also a major problem.

• Key values

These are more of a problem because these can contain new lines and are far more likely to be exploited as most users of this library will not allow arbitrary keys.

ini/file.go

Lines 358 to 366 in 3be5ad4

	// In case key value contains "\n", "`", "\"", "#" or ";"
	if strings.ContainsAny(val, "\n`") {
	val = `"""` + val + `"""`
	} else if !f.options.IgnoreInlineComment && strings.ContainsAny(val, "#;") {
	val = "`" + val + "`"
	}
	if _, err := buf.WriteString(equalSign + val + LineBreak); err != nil {
	return nil, err
	}

A value containing """\n can rewrite arbitrary sections of the config file.

e.g. the value

VALUE"""
[ANOTHER SECTION]
KEYS=VALUES
...
; more arbitrary content

; """

Do you have any suggestion to fix the problem?

If there is no proper mechanism for escaping these cases, (I suspect that in general that this is the case as ini files are very poorly documented - but I guess an option could given to backslash escape) go-ini should simply return an error saying that there is no safe delimiter.

Check that the chosen delimiter is not included in the value and not just assume that because one delimiter is included you can just use another without checking.

[Tim-Blyth-SiteHost]

I believe a test that covers the cases above should be added, which attempts to load the file using parse_ini_file via PHP.

Something like:

<?PHP
$ini_array = parse_ini_file("test.ini");
print_r($ini_array);

Where test.ini is generated via SaveTo and contains keys and values and sections with cases for all the above escape flaws.

I'm unsure if this fits every general case, but in some preliminary testing I have found that replacing the entire value escape section with golang stdlib val = strconv.Quote(val) escapes the values much better, with the only downside of not allowing newlines (newlines in the value string becomes the string \n) which depending on your use case may be desired, but could cause problems for uses where newlines are desired.

I imagine relying on a stdlib library, we are going to avoid a lot of the hard work and edgecase flaws we are seeing above.

[Tim-Blyth-SiteHost]

Or alternatively allow a no-escape option where it will use the string from val directly so we can do our own escaping before adding the value.

The issue I hit is we were using strconv.Quote before passing values to this library, but the value had a backtick (`) which meant """ was added to either side, leading us to a value with 4 quotes each side and an unparsable ini file

[Tim-Blyth-SiteHost]

If using strconv.Quote directly is not possible (as it escapes some characters that may not be desired), the source code would be a good guide to escaping characters https://github.com/golang/go/blob/master/src/strconv/quote.go

[Tim-Blyth-SiteHost]

Related: #332