Ceph » RADOS

The bad json data that crashed the mon is pasted below..

curl -k "$endpoint/request" -X POST -d '{"prefix":"auth add","entity":"client.testuser02","caps":"mon '\''allow r'\'' osd '\''allow rw pool=testpool01'\''"}'

[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Core was generated by `/usr/bin/ceph-mon -f --cluster ceph --id juju-8c5f4a-sts-stein-bionic-0 --setus'.
Program terminated with signal SIGABRT, Aborted.
#0  raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
51    ../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
[Current thread is 1 (Thread 0x7f40c85fd700 (LWP 71986))]
(gdb) bt
#0  raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00005570a1df13a0 in reraise_fatal (signum=6) at ./src/global/signal_handler.cc:81
#2  handle_fatal_signal (signum=6) at ./src/global/signal_handler.cc:326
#3  <signal handler called>
#4  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#5  0x00007f40d31337f1 in __GI_abort () at abort.c:79
#6  0x00007f40d3d45957 in ?? () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#7  0x00007f40d3d4bae6 in ?? () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#8  0x00007f40d3d4bb21 in std::terminate() () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#9  0x00007f40d3d4bd54 in __cxa_throw () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#10 0x00005570a1bea218 in ceph::common::cmd_getval<std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > (cmdmap=std::map with 3 elements = {...}, k="caps", val=std::vector of length 0, capacity 0) at ./src/common/cmdparse.h:71
#11 0x00005570a1b91add in Monitor::_generate_command_map (cmdmap=std::map with 3 elements = {...}, param_str_map=std::map with 0 elements) at ./src/mon/Monitor.cc:3031
#12 0x00005570a1bc4cee in Monitor::handle_command (this=this@entry=0x5570a452b000, op=...) at ./src/mon/Monitor.cc:3335
#13 0x00005570a1bcb0ea in Monitor::dispatch_op (this=this@entry=0x5570a452b000, op=...) at ./src/mon/Monitor.cc:4494
#14 0x00005570a1bcc2ca in Monitor::_ms_dispatch (this=this@entry=0x5570a452b000, m=m@entry=0x5570b7284280) at ./src/mon/Monitor.cc:4356
#15 0x00005570a1bfaab8 in Monitor::ms_dispatch (m=0x5570b7284280, this=0x5570a452b000) at ./src/mon/Monitor.h:885
#16 Dispatcher::ms_dispatch2 (this=0x5570a452b000, m=...) at ./src/msg/Dispatcher.h:124
#17 0x00007f40d53bad62 in Messenger::ms_deliver_dispatch (m=..., this=0x5570a452f800) at ./src/msg/Messenger.h:703
#18 DispatchQueue::entry (this=0x5570a452fb20) at ./src/msg/DispatchQueue.cc:199
#19 0x00007f40d545a25d in DispatchQueue::DispatchThread::entry (this=<optimized out>) at ./src/msg/DispatchQueue.h:101
#20 0x00007f40d34eb6db in start_thread (arg=0x7f40c85fd700) at pthread_create.c:463
#21 0x00007f40d321461f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

 (gdb) p cmdmap
$1 = std::map with 3 elements = {["caps"] = {which_ = 0, storage_ = {<boost::detail::aligned_storage::aligned_storage_imp<32, 8>> = {data_ = {
          buf = "\300\211\343\246pU\000\000,\000\000\000\000\000\000\000,", '\000' <repeats 14 times>, align_ = {<No data fields>}}}, static size = <optimized out>, static alignment = <optimized out>}}, 
  ["entity"] = {which_ = 0, storage_ = {<boost::detail::aligned_storage::aligned_storage_imp<32, 8>> = {data_ = {buf = " \234̩pU\000\000\021\000\000\000\000\000\000\000\036", '\000' <repeats 14 times>, 
          align_ = {<No data fields>}}}, static size = <optimized out>, static alignment = <optimized out>}}, ["prefix"] = {which_ = 0, 
    storage_ = {<boost::detail::aligned_storage::aligned_storage_imp<32, 8>> = {data_ = {buf = "hC\227\253pU\000\000\b\000\000\000\000\000\000\000auth add\000\000\000\000\000\000\000", 
          align_ = {<No data fields>}}}, static size = <optimized out>, static alignment = <optimized out>}}}
(gdb) p param_str_map
$2 = std::map with 0 elements
(gdb) 

(gdb) info locals
cv = std::vector of length 0, capacity 0
p = 
      {first = "caps", second = {which_ = 0, storage_ = {<boost::detail::aligned_storage::aligned_storage_imp<32, 8>> = {data_ = {buf = "\300\211\343\246pU\000\000,\000\000\000\000\000\000\000,", '\000' <repeats 14 times>, align_ = {<No data fields>}}}, static size = <optimized out>, static alignment = <optimized out>}}}
(gdb) 

I think we should validate the string in 

void Monitor::handle_command(MonOpRequestRef op)
{
  ceph_assert(op->is_type_command());
  auto m = op->get_req<MMonCommand>();
  if (m->fsid != monmap->fsid) {
    dout(0) << "handle_command on fsid " << m->fsid << " != " << monmap->fsid
            << dendl;
    reply_command(op, -EPERM, "wrong fsid", 0);
    return;
  }

(gdb) f 12
#12 0x00005570a1bc4cee in Monitor::handle_command (this=this@entry=0x5570a452b000, op=...) at ./src/mon/Monitor.cc:3335
3335    in ./src/mon/Monitor.cc

(gdb) info locals
__PRETTY_FUNCTION__ = "void Monitor::handle_command(MonOpRequestRef)" 
m = <optimized out>
session = <optimized out>
__func__ = "handle_command" 
prefix = "auth add" 
fullcmd = std::vector of length 2, capacity 2 = {"auth", "add"}
cmdmap = std::map with 3 elements = {["caps"] = {which_ = 0, storage_ = {<boost::detail::aligned_storage::aligned_storage_imp<32, 8>> = {data_ = {
          buf = "\300\211\343\246pU\000\000,\000\000\000\000\000\000\000,", '\000' <repeats 14 times>, align_ = {<No data fields>}}}, static size = <optimized out>, static alignment = <optimized out>}}, 
  ["entity"] = {which_ = 0, storage_ = {<boost::detail::aligned_storage::aligned_storage_imp<32, 8>> = {data_ = {buf = " \234̩pU\000\000\021\000\000\000\000\000\000\000\036", '\000' <repeats 14 times>, 
          align_ = {<No data fields>}}}, static size = <optimized out>, static alignment = <optimized out>}}, ["prefix"] = {which_ = 0, 
    storage_ = {<boost::detail::aligned_storage::aligned_storage_imp<32, 8>> = {data_ = {buf = "hC\227\253pU\000\000\b\000\000\000\000\000\000\000auth add\000\000\000\000\000\000\000", 
          align_ = {<No data fields>}}}, static size = <optimized out>, static alignment = <optimized out>}}}
ss = <incomplete type>
ds = <incomplete type>
rdata = {_buffers = {_root = {next = 0x7f40c85f93e0}, _tail = 0x7f40c85f93e0}, _carriage = 0x5570a23bb9c0 <ceph::buffer::v15_2_0::list::always_empty_bptr>, _len = 0, _num = 0, static always_empty_bptr = {
    _raw = 0x0, _off = 0, _len = 0}}
rs = "unrecognized command" 
r = -22
module = "auth" 
err = "" 
format = "plain" 
f = {px = 0x0}
leader_cmd = <optimized out>
mgr_cmds = <optimized out>
mgr_cmd = <optimized out>
mon_cmd = <optimized out>
service = "auth" 
cmd_is_rw = true
param_str_map = std::map with 0 elements
(gdb) 

Discussed with Brad Hubbard. It seems the caps should be a list. For eg, this json works fine,

curl -k -H "Authorization: Basic $TOKEN" "https://juju-3b3d82-10-lxd-0:8003/request" -X POST -d '{"prefix":"auth add","entity":"client.testuser02","caps":["mon", "allow r", "osd", "allow rw pool=testpool01"]}'

but this does not,

curl -k -H "Authorization: Basic $TOKEN" "https://juju-3b3d82-10-lxd-0:8003/request" -X POST -d '{"prefix":"auth add","entity":"client.testuser02","caps":"mon '\''allow r'\'' osd '\''allow rw pool=testpool01'\''"}'

A patch of this sort may work to avoid the abort, since we will catch the exception if we fail to populate cv from the caps and call reply_command() with the appropriate message.

diff --git a/src/mon/Monitor.cc b/src/mon/Monitor.cc
index b32af4ba809..f3e771034bc 100644
--- a/src/mon/Monitor.cc
+++ b/src/mon/Monitor.cc
@@ -3028,13 +3028,20 @@ void Monitor::_generate_command_map(cmdmap_t& cmdmap,
       continue;
     if (p->first == "caps") {
       vector<string> cv;
-      if (cmd_getval(cmdmap, "caps", cv) &&
-         cv.size() % 2 == 0) {
-       for (unsigned i = 0; i < cv.size(); i += 2) {
-         string k = string("caps_") + cv[i];
-         param_str_map[k] = cv[i + 1];
-       }
-       continue;
+      try
+      {
+        if (cmd_getval(cmdmap, "caps", cv) &&
+           cv.size() % 2 == 0) {
+         for (unsigned i = 0; i < cv.size(); i += 2) {
+           string k = string("caps_") + cv[i];
+           param_str_map[k] = cv[i + 1];
+         }
+         continue;
+        }
+      }
+      catch(...)
+      {
+        reply_command(op, -EINVAL, "Could not populate caps", 0);
       }
     }
     param_str_map[p->first] = cmd_vartype_stringify(p->second);

diff --git a/src/mon/Monitor.cc b/src/mon/Monitor.cc
index b32af4ba809..175ffa51ada 100644
--- a/src/mon/Monitor.cc
+++ b/src/mon/Monitor.cc
@@ -3028,13 +3028,20 @@ void Monitor::_generate_command_map(cmdmap_t& cmdmap,
       continue;
     if (p->first == "caps") {
       vector<string> cv;
-      if (cmd_getval(cmdmap, "caps", cv) &&
-         cv.size() % 2 == 0) {
-       for (unsigned i = 0; i < cv.size(); i += 2) {
-         string k = string("caps_") + cv[i];
-         param_str_map[k] = cv[i + 1];
-       }
-       continue;
+      try
+      {
+        if (cmd_getval(cmdmap, "caps", cv) &&
+           cv.size() % 2 == 0) {
+         for (unsigned i = 0; i < cv.size(); i += 2) {
+           string k = string("caps_") + cv[i];
+           param_str_map[k] = cv[i + 1];
+         }
+         continue;
+        }
+      }
+      catch(bad_cmd_get e)
+      {
+        reply_command(op, -EINVAL, e.what(), 0);
       }
     }

The earlier patch fails to compile, so I have changed it to the following, which does build.

diff --git a/src/mon/Monitor.cc b/src/mon/Monitor.cc
index b32af4ba809..73106494f68 100644
--- a/src/mon/Monitor.cc
+++ b/src/mon/Monitor.cc
@@ -3332,7 +3332,14 @@ void Monitor::handle_command(MonOpRequestRef op)

   // validate user's permissions for requested command
   map<string,string> param_str_map;
-  _generate_command_map(cmdmap, param_str_map);
+  try {
+    _generate_command_map(cmdmap, param_str_map);
+  }
+  catch(bad_cmd_get e)
+  {
+    reply_command(op, -EINVAL, e.what(), 0);
+  }
+
   if (!_allowed_command(session, service, prefix, cmdmap,
                         param_str_map, mon_cmd)) {
     dout(1) << __func__ << " access denied" << dendl;

I've tested the above patch, and it seems to be working as intended.

root@focal-testing:/home/nikhil/Downloads/ceph_upstream/ceph/build# ceph -s
2022-03-22T04:46:26.671+0000 7f32384c2700 -1 WARNING: all dangerous and experimental features are enabled.
2022-03-22T04:46:26.763+0000 7f32384c2700 -1 WARNING: all dangerous and experimental features are enabled.
  cluster:
    id:     cc29048a-b441-4d8c-9c35-2ede3188c292
    health: HEALTH_OK

  services:
    mon: 3 daemons, quorum a,b,c (age 21m)
    mgr: x(active, since 20m)
    mds: a:1 {0=a=up:active}
    osd: 3 osds: 3 up (since 19m), 3 in (since 19m)

  data:
    pools:   3 pools, 65 pgs
    objects: 22 objects, 2.2 KiB
    usage:   6.0 GiB used, 297 GiB / 303 GiB avail
    pgs:     65 active+clean

---

root@focal-testing:/home/nikhil/Downloads/ceph_upstream/ceph/build# cat test_ceph.sh 
#!/usr/bin/env python3
import json
import rados
c = rados.Rados(conffile='ceph.conf')
c.connect()
#cmd = json.dumps({"prefix":"auth add","entity":"client.testuser02","caps":"mon '\''allow r'\'' osd '\''allow rw pool=testpool01'\''"})
cmd = json.dumps({"prefix":"auth add","entity":"client.testuser02","caps":["mon", "allow r", "osd", "allow rw pool=testpool01"]})
print(c.mon_command(cmd, b''))

root@focal-testing:/home/nikhil/Downloads/ceph_upstream/ceph/build# ./test_ceph.sh 
(0, b'', 'added key for client.testuser02')

root@focal-testing:/home/nikhil/Downloads/ceph_upstream/ceph/build# cat test_ceph.sh 
#!/usr/bin/env python3
import json
import rados
c = rados.Rados(conffile='ceph.conf')
c.connect()
cmd = json.dumps({"prefix":"auth add","entity":"client.testuser02","caps":"mon '\''allow r'\'' osd '\''allow rw pool=testpool01'\''"})
#cmd = json.dumps({"prefix":"auth add","entity":"client.testuser02","caps":["mon", "allow r", "osd", "allow rw pool=testpool01"]})
print(c.mon_command(cmd, b''))
root@focal-testing:/home/nikhil/Downloads/ceph_upstream/ceph/build# ./test_ceph.sh 
(-22, b'', "bad or missing field 'caps'")

root@focal-testing:/home/nikhil/Downloads/ceph_upstream/ceph/build# ceph -s
2022-03-22T04:50:52.789+0000 7f56fef4f700 -1 WARNING: all dangerous and experimental features are enabled.
2022-03-22T04:50:52.801+0000 7f56fef4f700 -1 WARNING: all dangerous and experimental features are enabled.
  cluster:
    id:     cc29048a-b441-4d8c-9c35-2ede3188c292
    health: HEALTH_OK

  services:
    mon: 3 daemons, quorum a,b,c (age 25m)
    mgr: x(active, since 25m)
    mds: a:1 {0=a=up:active}
    osd: 3 osds: 3 up (since 23m), 3 in (since 23m)

  data:
    pools:   3 pools, 65 pgs
    objects: 22 objects, 2.2 KiB
    usage:   6.0 GiB used, 297 GiB / 303 GiB avail
    pgs:     65 active+clean

Could you please send a PR for this?

Thanks for the comment Radoslaw!

This reproduces on master as well. Unfortunately I do not seem to have permissions to "edit" the bug once created, so I cannot change it from Ceph - v15.2.14 to master.

I've sent the PR - https://github.com/ceph/ceph/pull/45547

Reproduces on master.

https://github.com/ceph/ceph/pull/45547 merged

I don't think https://github.com/ceph/ceph/pull/45547 is a complete fix, see my comment in the PR.

Ilya Dryomov wrote:

I don't think https://github.com/ceph/ceph/pull/45547 is a complete fix, see my comment in the PR.

I submitted https://github.com/ceph/ceph/pull/48044 to bail out of handle_command if an exception is thrown.

nikhil kshirsagar wrote:

Ilya Dryomov wrote:

I don't think https://github.com/ceph/ceph/pull/45547 is a complete fix, see my comment in the PR.

I submitted https://github.com/ceph/ceph/pull/48044 to bail out of handle_command if an exception is thrown.

Created https://tracker.ceph.com/issues/57859 for the cleanup, and linked it in https://github.com/ceph/ceph/pull/48044