Kibana allows users to create a saved object with an empty ID

[jportner]

Kibana version: 7.11.0 - 7.16.1

Describe the bug:

Credit to @wwang500 for discovering this bug with a fuzzing tool!

When you create a saved object in Kibana, if you don't specify an ID, it is supposed to automatically generate one. Before 7.11, we relied on Elasticsearch do to this (using the create document API instead of the index document API). However, starting in #84113, we introduced a change to use uuidv4 to generate an object ID before calling Elasticsearch; the reason for this is so that we can log a complete audit trail of events.

However, this change introduced a regression that allowed an empty object ID ("") to be assigned when the object is created. When Kibana serializes a saved object to an ES document, its raw document ID is formatted as <type>:<id>. When Kibana later tries to deserialize the document back into a saved object, it validates the raw ID. If the raw ID is malformed (such as <type>:), Kibana throws an error.

Note, it is only possible to get into this situation using the SavedObjectsClient create API, not bulkCreate.

Steps to reproduce:

1. Start Kibana and Elasticsearch
2. Create a malformed saved object:

   curl -X POST -u elastic:changeme "http://localhost:5601/api/saved_objects/index-pattern/" -H 'kbn-xsrf: true' -H 'Content-Type: application/json' -d'
   {
     "attributes": {
       "title": "my-pattern-*"
     }
   }
   '
   
   {"statusCode":500,"error":"Internal Server Error","message":"An internal server error occurred."}
   

3. Navigate to the Saved Objects Management page. If you don't see an error right away, the malformed object might not be loaded on the first page, in this case filter for "Data views" and you should see the error:
   
4. Observe the error message in the Kibana server logs:

   [2021-11-17T17:21:33.496-05:00][ERROR][http] Error: Raw document 'index-pattern:' does not start with expected prefix 'index-pattern:'
       at SavedObjectsSerializer.checkIsRawSavedObject (/Users/joe/GitHub/kibana-5/src/core/server/saved_objects/serialization/serializer.ts:68:13)
       at SavedObjectsSerializer.rawToSavedObject (/Users/joe/GitHub/kibana-5/src/core/server/saved_objects/serialization/serializer.ts:83:10)
       at SavedObjectsRepository._rawToSavedObject (/Users/joe/GitHub/kibana-5/src/core/server/saved_objects/service/lib/repository.ts:2073:42)
       at map (/Users/joe/GitHub/kibana-5/src/core/server/saved_objects/service/lib/repository.ts:981:19)
       at Array.map (<anonymous>)
       at SavedObjectsRepository.find (/Users/joe/GitHub/kibana-5/src/core/server/saved_objects/service/lib/repository.ts:978:37)
       at runMicrotasks (<anonymous>)
       at processTicksAndRejections (node:internal/process/task_queues:96:5)
       at SavedObjectsClient.find (/Users/joe/GitHub/kibana-5/src/core/server/saved_objects/service/saved_objects_client.ts:487:12)
       at EncryptedSavedObjectsClientWrapper.find (/Users/joe/GitHub/kibana-5/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts:171:7)
       at SecureSavedObjectsClientWrapper.find (/Users/joe/GitHub/kibana-5/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts:263:22)
       at SpacesSavedObjectsClient.find (/Users/joe/GitHub/kibana-5/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts:158:12)
       at /Users/joe/GitHub/kibana-5/src/plugins/saved_objects_management/server/routes/find.ts:73:28
       at /Users/joe/GitHub/kibana-5/src/core/server/http/router/error_wrapper.ts:15:14
       at Router.handle (/Users/joe/GitHub/kibana-5/src/core/server/http/router/router.ts:275:30)
       at handler (/Users/joe/GitHub/kibana-5/src/core/server/http/router/router.ts:230:13)
       at exports.Manager.execute (/Users/joe/GitHub/kibana-5/node_modules/@hapi/hapi/lib/toolkit.js:60:28)
       at Object.internals.handler (/Users/joe/GitHub/kibana-5/node_modules/@hapi/hapi/lib/handler.js:46:20)
       at exports.execute (/Users/joe/GitHub/kibana-5/node_modules/@hapi/hapi/lib/handler.js:31:20)
       at Request._lifecycle (/Users/joe/GitHub/kibana-5/node_modules/@hapi/hapi/lib/request.js:371:32)
       at Request._execute (/Users/joe/GitHub/kibana-5/node_modules/@hapi/hapi/lib/request.js:281:9)
   

Any other API call in Kibana that tries to load the saved object will also fail in this spectacular fashion.

Expected behavior:

Kibana should not allow users to create a saved object with an empty ID.

[elasticmachine]

Pinging @elastic/kibana-core (Team:Core)

[elasticmachine]

Pinging @elastic/kibana-security (Team:Security)

[pgayvallet]

@jportner is that a duplicate(-ish) of #105039?

[jportner]

@jportner is that a duplicate(-ish) of #105039?

Yeah, I forgot about that one.
I think we can fix the empty-ID problem by treating id: "" as equivalent to id: undefined. In that case, we would just generate a random object ID. That's functionally what we already do in bulkCreate, and that's what we used to do in create prior to 7.11. So this feels less like a breaking change and more like a bugfix.

IMO, #105039 is a bit bigger in scope as actual new validation would be a breaking change.

I'm still trying to think of what to do about update+upsert, though...

[pgayvallet]

In that case, we would just generate a random object ID. That's functionally what we already do in bulkCreate, and that's what we used to do in create prior to 7.11. So this feels less like a breaking change and more like a bugfix.

That would make sense

I'm still trying to think of what to do about update+upsert, though...

Can you elaborate on this one?

[jportner]

Kibana behaves this way today:

a. bulkCreate w/ id: undefined -> generate a random object ID
b. create w/ id: undefined -> generate a random object ID
c. bulkCreate w/ id: '' -> generate a random object ID
d. create w/ id: '' -> create a broken object with ID ''

Since action (d) before 7.11 didn't behave this way -- it also generated a random object ID -- it makes sense to me to treat this like a bug and change that behavior back to how it was.

Now, we have other scenarios to consider:

e. update w/ id: undefined -> TS error (id is required)
f. update w/ id: undefined, upsert: true -> TS error (id is required)
g. update w/ id: '' -> 404 error
h. update w/ id: '', upsert: true -> create a broken object with ID ''

Action (h) is also problematic, but how should we approach it? If we treat this as a 400 Bad Request, then we should probably respond to action (g) the same way.

FWIW I just tested our generic update API and it's not possible to trigger an update with an empty object ID this way, you'll just get a 404 error. I guess this is happening because the update route has path: '/{type}/{id}' which doesn't allow for an empty string, whereas the create route has path: '/{type}/{id?}' which does allow for an empty string. TIL.

So maybe we don't need to worry about update+upsert for now, since it sounds like there's a much lower chance someone could accidentally create an object with an empty ID using this API.

[jportner]

I think for any update with an empty ID (e/f/g/h above), we should just throw a Bad Request error.

[huynholivier]

I faced this issue while trying to import a dashboard ndjson file with an empty ID , now the saved object, and dashboard menus are broken, actually I'm using a version 7.16, can you tell me how I can fix this issue ?