Added activity logging for ext APIs with custom bindings

chrome/common/extensions/extension_messages.h

Index: chrome/common/extensions/extension_messages.h
diff --git a/chrome/common/extensions/extension_messages.h b/chrome/common/extensions/extension_messages.h
index c3eb6fd14eaedb5ecd6ba7822f45651e3d05e85b..431b2a1927c883bf0a36371d0d8f869d51c73a9d 100644
--- a/chrome/common/extensions/extension_messages.h
+++ b/chrome/common/extensions/extension_messages.h
@@ -30,6 +30,18 @@
 
 IPC_ENUM_TRAITS(chrome::ViewType)
 
+// Parameters structure for ExtensionHostMsg_AddAPIActionToActivityLog.
+IPC_STRUCT_BEGIN(ExtensionHostMsg_APIAction_Params)

[palmer, 2013/03/18 20:58:10]

This is a rich API; taking a DeepCopy of the arguments and so on. Cold we put
this behind a flag (potentially manipulable with the Finch experiments
framework) at first? I don't feel super comfortable having this enabled on all
Chrome installations right away.

[felt, 2013/03/18 21:01:33]

What are your concerns?  Perhaps I can address them.  (FYI: this is almost
identical to the existing ExtensionHostMsg_AddDOMAPIActionToActivityLog, but
with different parameters.)

I can put it behind the ActivityLog flag so that IPCs are only set if the
ActivityLog is enabled.

On 2013/03/18 20:58:10, Chris P. wrote:

[palmer, 2013/03/18 21:22:13]

I don't see an obvious, immediate vulnerability, but we are skittish about
powerful general-purpose IPC APIs because we've gotten bitten a few times. I
added ExtensionHostMsg_AddDOMAPIActionToActivityLog to our spreadsheet of IPCs
to audit. :)

In this case, the information flows from renderer to browser, 
Gating it on whether or not the ActivityLog is enabled sounds good, but the
place to enforce it (for security purposes) is on the receiving side: the
receiver should immediately ignore incoming IPC messages unless ActivityLog is
enabled. (Of course, at that point it also makes sense for the IPC client to
check the setting before sending the message.) I see in
chrome/browser/extensions/activity_log.cc that it does return immediately if
!IsLogEnabled(), but it might be more efficient to shut the process down sooner
in the call tree.

I'm not super certain that there is undue risk here. CCing jschuh who has more
experience here. Thoughts, Justin?

[felt, 2013/03/18 23:34:19]

OK, I added an additional check in
chrome/browser/renderer_host/chrome_render_message_filter.cc that simply returns
if the ActivityLog is not turned on. This is one method call closer to the
originating IPC.

On 2013/03/18 21:22:13, Chris P. wrote:

+  // API name.
+  IPC_STRUCT_MEMBER(std::string, api_call)
+
+  // List of arguments.
+  IPC_STRUCT_MEMBER(ListValue, arguments)
+
+  // Extra logging information.
+  IPC_STRUCT_MEMBER(std::string, extra)
+IPC_STRUCT_END()
+
 // Parameters structure for ExtensionHostMsg_AddDOMActionToActivityLog.
 IPC_STRUCT_BEGIN(ExtensionHostMsg_DOMAction_Params)
   // URL of the page.
@@ -588,7 +600,12 @@ IPC_MESSAGE_CONTROL1(ExtensionHostMsg_ResumeRequests, int /* route_id */)
 IPC_MESSAGE_ROUTED1(ExtensionHostMsg_UpdateDraggableRegions,
                     std::vector<extensions::DraggableRegion> /* regions */)
 
-// Sent by the renderer to log a DOM action on the extension activity log.
+// Sent by the renderer to log an API action to the extension activity log.
+IPC_MESSAGE_CONTROL2(ExtensionHostMsg_AddAPIActionToActivityLog,
+                     std::string /* extension_id */,
+                     ExtensionHostMsg_APIAction_Params)
+
+// Sent by the renderer to log a DOM action to the extension activity log.
 IPC_MESSAGE_CONTROL2(ExtensionHostMsg_AddDOMActionToActivityLog,
                      std::string /* extension_id */,
                      ExtensionHostMsg_DOMAction_Params)