Merge lp:~wlxing/ecryptfs/fix-bug-1678689 into lp:ecryptfs

[Jason Xing]

This revision(r886) is linked to #bug1678689.

Error Case: In the original version, users cannot mount ecryptfs with "-o passphrase_passwd_file=mykey" (<mykey> is "passwd=123=abc") or "-o passwd=123=abc". Because the passphrase user uses includes "=" character.

Error Hint:
http://bazaar.launchpad.net/~ecryptfs/ecryptfs/trunk/view/head:/src/libecryptfs/cmd_ln_parser.c#L290

Explanation:
Because in the manner of process_comma_tok() function (in the src/libecryptfs/cmd_in_parsers.c), ecryptfs will store "passwd=123" as the @current(parameter in that function)->name and "abc" as @current->value. That will go wrong when we go into tf_pass_file() function (in src/key_mod/ecryptfs_key_mod_passphrase.c) because it cannot match the name "passphrase_passwd" or "passwd".

Patch:
Thus, I add two lines in process_comma_tok() to change that case. If we match one "=" character in the string, we don't need to loop and match another "=" again. More details in #change of my branch.

[Tyler Hicks (tyhicks)]

Hi Jason - Thanks for the fix!

As you know, the code that you're patching breaks "name=value" pairs up into individual parts. The 'i' variable is used to represent the length of the "name" portion of the string.

I don't think it is correct to set i equal to st_len as st_len is the length of the entire "name=value" string. IIUC, simply adding a break would fix the bug.

What was your reasoning for setting i to st_len?

review: Needs Information

[Jason Xing (wlxing)]

Hi Tyler,

Thanks for your check.

Some test cases without "i = st_len":
1) If you add other options like "-o key=passphrase", It would cause
the problem saying "Error attempting to evaluate mount options: [-14]
Bad address...".
2) If you add other options like "-o key=passphrase", It would cause
the problem saying "Error attempting to evaluate mount options: [-22]
Invalid argument...".

Explanation:
If you look at this line "if ((i-j) > 1) {" (it is just several lines
below the break code), you will understand that "i" here means the
length of the whole separate option(eg. key=passpharse). If I just add
break without setting i to st_len, it will skip this line "if ((i-j) >
1) {" and will not allocate any memory for "value".

Does it make sense?

Jason

On Fri, May 26, 2017 at 12:21 AM, Tyler Hicks <email address hidden> wrote:
> Review: Needs Information
>
> Hi Jason - Thanks for the fix!
>
> As you know, the code that you're patching breaks "name=value" pairs up into individual parts. The 'i' variable is used to represent the length of the "name" portion of the string.
>
> I don't think it is correct to set i equal to st_len as st_len is the length of the entire "name=value" string. IIUC, simply adding a break would fix the bug.
>
> What was your reasoning for setting i to st_len?
> --
> https://code.launchpad.net/~wlxing/ecryptfs/fix-bug-1678689/+merge/321653
> You are the owner of lp:~wlxing/ecryptfs/fix-bug-1678689.

[Jason Xing (wlxing)]

Update
"1) If you add other options like "-o key=passphrase", It would cause
the problem saying "Error attempting to evaluate mount options: [-14]
Bad address..."."

Correct sentence:
"1) If you add other options like "-o passphrase_passwd_file=mykey", It would cause
the problem saying "Error attempting to evaluate mount options: [-14]
Bad address..."."

[Tyler Hicks (tyhicks)]

Thanks for the explanation! You've convinced me that I was wrong about the intent of the "i" variable. I've merged this change.

review: Approve

=== modified file 'src/libecryptfs/cmd_ln_parser.c'
--- src/libecryptfs/cmd_ln_parser.c	2015-02-13 13:59:54 +0000
+++ src/libecryptfs/cmd_ln_parser.c	2017-04-03 03:03:45 +0000
@@ -297,6 +297,8 @@
 			memcpy(name, sub_token, i);
 			name[i] = '\0';
 			j = i;
+			i = st_len;
+			break;
 		}
 	if (!name) {
 		if (!(name = malloc(i+1))) {