eCryptfs

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

fix-bug-1678689
Merge into trunk

Proposed by Jason Xing on 2017-04-03

Status:	Merged
Merged at revision:	886
Proposed branch:	lp://staging/~wlxing/ecryptfs/fix-bug-1678689
Merge into:	lp://staging/ecryptfs
Diff against target:	12 lines (+2/-0) 1 file modified src/libecryptfs/cmd_ln_parser.c (+2/-0)
To merge this branch:	bzr merge lp://staging/~wlxing/ecryptfs/fix-bug-1678689
Related bugs:	Link a bug report

Reviewer	Date Requested	Status
Tyler Hicks	2017-05-24	Approve on 2017-05-25
Michal Hlavinka	2017-04-03	Pending
Review via email: mp+321653@code.staging.launchpad.net

Description of the change

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.

Revision history for this message

Tyler Hicks (tyhicks) wrote on 2017-05-25:

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

Revision history for this message

Jason Xing (wlxing) wrote on 2017-05-25:

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.

Revision history for this message

Jason Xing (wlxing) wrote on 2017-05-25:

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..."."