New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 877711 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 28
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

"cryptohome --action=remove" tries to decode null reply after failed D-Bus call

Project Member Reported by derat@chromium.org, Aug 24

Issue description

Tast tests that log in to Chrome are sometimes failing on betty-release. From http://cros-goldeneye/chromeos/healthmonitoring/buildDetails?buildbucketId=8937319159574725584:

2018/08/24 08:34:35 Started test arc.Boot
2018/08/24 08:34:35 [10:34:35.644] Restarting ui job
2018/08/24 08:34:37 [10:34:36.959] Waiting for org.chromium.SessionManager D-Bus service
2018/08/24 08:34:37 [10:34:36.970] Asking session_manager to enable Chrome testing
2018/08/24 08:34:37 [10:34:36.974] Waiting for Chrome to write its debugging port to /home/chronos/DevToolsActivePort
2018/08/24 08:34:38 [10:34:37.722] Removing cryptohome for testuser@gmail.com
2018/08/24 08:34:38 [10:34:37.880] Error at boot.go:33: Failed to connect to Chrome: signal: segmentation fault (core dumped) ()

The test is running "cryptohome --action=remove --force --user=<hash>". It looks like the cryptohome process (i.e. the client) is segfaulting; I see two crashes at http://pantheon/storage/browser/chromeos-image-archive/betty-release/R70-10999.0.0/tast_vm_test_results_2/tast_vm_canary/crashes/

Crash reason:  SIGSEGV /0x00000000
Crash address: 0x0
Process uptime: not available

Thread 0 (crashed)
 0  cryptohome!main [cryptohome.cc : 428 + 0x0]
    rax = 0x0000000000000000   rdx = 0x000000000000005d
    rcx = 0x00007bc8c2d348f0   rbx = 0x00007ffddb4d0030
    rsi = 0x00007bc8c2d348f0   rdi = 0x00007ffddb4d0038
    rbp = 0x00007ffddb4d0260   rsp = 0x00007ffddb4cfe80
     r8 = 0x00007bc8c2d317c0    r9 = 0x000000000000005d
    r10 = 0x0000000000000047   r11 = 0x000057c513301050
    r12 = 0x000057c513312a10   r13 = 0x000057c513301d80
    r14 = 0x0000000000000000   r15 = 0x0000000000000001
    rip = 0x000057c511a8a55c
    Found by: given as instruction pointer in context
 1  libc-2.23.so!__libc_start_main [libc-start.c : 289 + 0x1a]
    rbx = 0x0000000000000000   rbp = 0x00007ffddb4d0330
    rsp = 0x00007ffddb4d0270   r12 = 0x000057c511b44820
    r13 = 0x00007ffddb4d0350   r14 = 0x0000000000000000
    r15 = 0x0000000000000000   rip = 0x00007bc8c29ae736
    Found by: call frame info
 2  cryptohome!_start + 0x29
    rbx = 0x0000000000000000   rbp = 0x0000000000000000
    rsp = 0x00007ffddb4d0340   r12 = 0x000057c511a83ef0
    r13 = 0x00007ffddb4d0350   r14 = 0x0000000000000000
    r15 = 0x0000000000000000   rip = 0x000057c511a83f19
    Found by: call frame info
 3  0x7ffddb4d0348
    rbx = 0x0000000000000000   rbp = 0x0000000000000000
    rsp = 0x00007ffddb4d0348   r12 = 0x000057c511a83ef0
    r13 = 0x00007ffddb4d0350   r14 = 0x0000000000000000
    r15 = 0x0000000000000000   rip = 0x00007ffddb4d0348
    Found by: call frame info
 4  0x7ffddb4e7000
    rsp = 0x00007ffddb4d03f0   rip = 0x00007ffddb4e7000
    Found by: stack scanning
 5  cryptohome + 0x18ef0
    rsp = 0x00007ffddb4d0480   rip = 0x000057c511a83ef0
    Found by: stack scanning

Here's the crashing code:

 425 void ParseBaseReply(GArray* reply_ary, cryptohome::BaseReply* reply) {
 426   if (!reply)
 427     return;
 428   if (!reply->ParseFromArray(reply_ary->data, reply_ary->len)) {
 429     printf("Failed to parse reply.\n");
 430     exit(1);
 431   }
 432   reply->PrintDebugString();
 433 }

This looks wrong:

1375     GArray* out_reply = nullptr;
1376     brillo::glib::ScopedError error;
1377     if (!org_chromium_CryptohomeInterface_remove_ex(
1378             proxy.gproxy(), account_ary.get(), &out_reply,
1379             &brillo::Resetter(&error).lvalue())) {
1380       printf("Remove call failed: %s.\n", error->message);
1381     }
1382 
1383     cryptohome::BaseReply reply;
1384     ParseBaseReply(out_reply, &reply);
1385     if (reply.has_error()) {
1386       printf("Remove failed.\n");
1387       return 1;
1388     }

Probably the intent was to return 1 after line 1380. The current code tries to decode the reply even if the D-Bus call fails.

(I haven't looked into why the D-Bus call is failing yet.)
 
betty-release%2FR70-10999.0.0%2Ftast_vm_test_results_2%2Ftast_vm_canary%2Fcrashes%2Fcryptohome.20180824.103437.2723.dmp.txt
4.7 KB View Download
Cc: ejcaruso@chromium.org jorgelo@chromium.org wad@chromium.org
Labels: -Pri-2 Restrict-View-SecurityTeam Pri-1
cryptohome.cc needs a lot of help. Its D-Bus error handling seems severely broken; I see a bunch of spots where it looks like it's reading uninitialized memory after calls fail.
Sent https://crrev.com/c/1188801 for review.
Re #2: sounds like the right CL.

Not sure if we need to have a Restrict-View-SecurityTeam label here. cryptohome is a CLI utility, and is not used for user logins etc during normal chromebook operation. It is only called by tests or manually for debug/testing purposes. So, it's crash is not a security concern. Chrome talks to cryptohomed directly over dbus, not using cryptohome CLI.
Labels: -Restrict-View-SecurityTeam
Thanks, makes sense.
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 28

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/2f1122fbfd60787a2caf0e5a1f32bc160935aba8

commit 2f1122fbfd60787a2caf0e5a1f32bc160935aba8
Author: Daniel Erat <derat@chromium.org>
Date: Tue Aug 28 03:54:08 2018

cryptohome: Improve handling of D-Bus errors.

Update cryptohome.cc to report failure after D-Bus method
calls fail.

BUG= chromium:877711 
TEST=unit tests pass (although i don't think that
     cryptohome.cc has any); also verified that login still
     works

Change-Id: I7909ced57c96ff1baaa93bbdc4c27f7ad72cf818
Reviewed-on: https://chromium-review.googlesource.com/1188801
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Andrey Pronin <apronin@chromium.org>

[modify] https://crrev.com/2f1122fbfd60787a2caf0e5a1f32bc160935aba8/cryptohome/cryptohome.cc

Status: Fixed (was: Started)

Sign in to add a comment