eCryptfs

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

  1. fix-bug-1678689
  2. Merge into trunk
Proposed by Jason Xing
Status: Merged
Merged at revision: 886
Proposed branch: lp:~wlxing/ecryptfs/fix-bug-1678689
Merge into: lp: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:~wlxing/ecryptfs/fix-bug-1678689
Reviewer Review Type Date Requested Status
Tyler Hicks Approve
Michal Hlavinka Pending
Review via email: mp+321653@code.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.

To post a comment you must log in.
Revision history for this message
Tyler Hicks (tyhicks) wrote :

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 :

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 :

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

Revision history for this message
Tyler Hicks (tyhicks) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/libecryptfs/cmd_ln_parser.c'
2--- src/libecryptfs/cmd_ln_parser.c 2015-02-13 13:59:54 +0000
3+++ src/libecryptfs/cmd_ln_parser.c 2017-04-03 03:03:45 +0000
4@@ -297,6 +297,8 @@
5 memcpy(name, sub_token, i);
6 name[i] = '\0';
7 j = i;
8+ i = st_len;
9+ break;
10 }
11 if (!name) {
12 if (!(name = malloc(i+1))) {

Subscribers

People subscribed via source and target branches