Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
235623002:
Password manager internals page: Improve security (Closed)
|
Created:
6 years, 8 months ago by vabr (Chromium) Modified:
3 years, 10 months ago CC:
chromium-reviews, benquan, jam, browser-components-watch_chromium.org, darin-cc_chromium.org, arv+watch_chromium.org, dyu1, estade+watch_chromium.org, Ilya Sherman, rouslan+autofillwatch_chromium.org, Dane Wallinga Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPassword manager internals page: Improve security
This CL makes sure that gathered logs are sanitized always on the browser side.
It is a follow-up to https://codereview.chromium.org/228263004/, where some security concerns were mentioned.
The security and other parts of the design doc has been updated: https://docs.google.com/document/d/1ArDhTo0w-8tOPiTwqM1gG6ZGqODo8UTpXlJjjnCNK4s/edit#heading=h.5eksqdbff8b
BUG=347927
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264949
Patch Set 1 : #
Total comments: 2
Patch Set 2 : innerHTML -> innerText #
Total comments: 11
Patch Set 3 : Splitting off HTML escaping to https://codereview.chromium.org/236203003 #Patch Set 4 : Rebased #Patch Set 5 : ID->string in a switch statement; also clang-format #Patch Set 6 : Sanitize before IPC and rebased #
Total comments: 10
Patch Set 7 : Comments addressed #Patch Set 8 : Compile errors fixed #
Total comments: 2
Patch Set 9 : Removed excessive logging functions #
Total comments: 4
Patch Set 10 : Pass a string16 by a const ref #
Messages
Total messages: 36 (0 generated)
Hi Ilya and Tom. Please take a look at how I tried to address the security concerns from yesterday. One particular thing: I did not use the resources for the fixed strings, because those strings are not expected to be translated. Instead, I implemented a map from string IDs to strings. If you feel it is still a better idea to use the resources, please let me know. Thanks! Vaclav
https://codereview.chromium.org/235623002/diff/20001/chrome/browser/resources... File chrome/browser/resources/password_manager_internals/password_manager_internals.js (right): https://codereview.chromium.org/235623002/diff/20001/chrome/browser/resources... chrome/browser/resources/password_manager_internals/password_manager_internals.js:12: textDiv.innerHTML = logText; Oh, not so good. How can we avoid this?
https://codereview.chromium.org/235623002/diff/20001/chrome/browser/resources... File chrome/browser/resources/password_manager_internals/password_manager_internals.js (right): https://codereview.chromium.org/235623002/diff/20001/chrome/browser/resources... chrome/browser/resources/password_manager_internals/password_manager_internals.js:12: textDiv.innerHTML = logText; On 2014/04/11 18:57:43, Tom Sepez wrote: > Oh, not so good. How can we avoid this? I can revert it to innerText without much troubles (I'm not too concerned with a potential ugliness of the output). My thoughts were that since the renderer is already not trusted and we need to ensure escaping the string in the browser, using innerText here would be just a theatre. Am I mistaken?
That's not a robust design against future changes which might not escape the string properly.
Tom, I put the innerText back. Please take a look again. Thanks! Vaclav
LGTM from a security perspective. Thanks.
https://codereview.chromium.org/235623002/diff/60001/chrome/browser/password_... File chrome/browser/password_manager/chrome_password_manager_client.h (right): https://codereview.chromium.org/235623002/diff/60001/chrome/browser/password_... chrome/browser/password_manager/chrome_password_manager_client.h:57: autofill::SavePasswordProgressLogger::StructuredLog>& logs) OVERRIDE; Wow, is this really how clang-format formats this line? That's not great :/ https://codereview.chromium.org/235623002/diff/60001/components/autofill/core... File components/autofill/core/common/save_password_progress_logger.h (right): https://codereview.chromium.org/235623002/diff/60001/components/autofill/core... components/autofill/core/common/save_password_progress_logger.h:40: // file. Rather than requiring the order to match, please just use a switch stmt with cases. It's a little longer, but it's also more robust. https://codereview.chromium.org/235623002/diff/60001/components/autofill/core... components/autofill/core/common/save_password_progress_logger.h:120: }; I'm not convinced that passing a struct rather than a string across the process boundary improves security in any way. I think it's sufficient to just replace the string "label" parameters with StringIDs instead. If either of you disagree, could you please convey what security or privacy threat the StructuredLog class prevents, over and above the guarantees made by the LogFoo() methods below? https://codereview.chromium.org/235623002/diff/60001/components/autofill/core... components/autofill/core/common/save_password_progress_logger.h:133: const std::vector<StructuredLog>& logs); How about only making this function available in the browser process?
Also, I'd appreciate it if you could split this CL up into two pieces: One for sending data to the WebUI page, and one for improving the security guarantees of the IPC message. Currently, the WebUI parts of the CL are very much buried in the midst of the StructuredLog changes.
Thanks Tom and Ilya, for your comments. I have yet to answer a couple of Ilya's comments, but we need to reach an understanding first on what should be sent over the IPC. Tom, it would be great if you could chime in to the discussion about the format of the IPC message below, as any changes to that would need a re-review by you anyway. Thanks, Vaclav https://codereview.chromium.org/235623002/diff/60001/chrome/browser/password_... File chrome/browser/password_manager/chrome_password_manager_client.h (right): https://codereview.chromium.org/235623002/diff/60001/chrome/browser/password_... chrome/browser/password_manager/chrome_password_manager_client.h:57: autofill::SavePasswordProgressLogger::StructuredLog>& logs) OVERRIDE; On 2014/04/12 00:00:51, Ilya Sherman wrote: > Wow, is this really how clang-format formats this line? That's not great :/ I can confirm this was clang-formated. Have you suggestions for a better split? I actually don't see any, the type name got monstrously long. https://codereview.chromium.org/235623002/diff/60001/components/autofill/core... File components/autofill/core/common/save_password_progress_logger.h (right): https://codereview.chromium.org/235623002/diff/60001/components/autofill/core... components/autofill/core/common/save_password_progress_logger.h:120: }; On 2014/04/12 00:00:51, Ilya Sherman wrote: > I'm not convinced that passing a struct rather than a string across the process > boundary improves security in any way. I think it's sufficient to just replace > the string "label" parameters with StringIDs instead. > > If either of you disagree, could you please convey what security or privacy > threat the StructuredLog class prevents, over and above the guarantees made by > the LogFoo() methods below? My understanding is that LogFoo don't actually present any guarantees, as long as they can be called from the (untrusted) renderer. If I understand you correctly, you suggest passing a StringID for a label and a string for the data through the IPC? If so, then it will be hardly possible to make sure on the browser side, that, e.g., a password or a sensitive URL is not included in the string.
https://codereview.chromium.org/235623002/diff/60001/components/autofill/core... File components/autofill/core/common/save_password_progress_logger.h (right): https://codereview.chromium.org/235623002/diff/60001/components/autofill/core... components/autofill/core/common/save_password_progress_logger.h:120: }; On 2014/04/12 09:11:38, vabr (Chromium) wrote: > On 2014/04/12 00:00:51, Ilya Sherman wrote: > > I'm not convinced that passing a struct rather than a string across the > process > > boundary improves security in any way. I think it's sufficient to just > replace > > the string "label" parameters with StringIDs instead. > > > > If either of you disagree, could you please convey what security or privacy > > threat the StructuredLog class prevents, over and above the guarantees made by > > the LogFoo() methods below? > > My understanding is that LogFoo don't actually present any guarantees, as long > as they can be called from the (untrusted) renderer. > > If I understand you correctly, you suggest passing a StringID for a label and a > string for the data through the IPC? > If so, then it will be hardly possible to make sure on the browser side, that, > e.g., a password or a sensitive URL is not included in the string. I would keep the previous IPC design, i.e. just send a string. However, I would keep the StringID changes for the LogFoo() methods in this class. With this design, it's not possible to write code that accidentally emits a password or a sensitive URL, other than by going through some contortions to, say, pass the password as the name_or_id param to LogHTMLForm(). That is, there's no chance that I as a developer might add code to log a password, unless I really abuse the API... but if I'm really abusing the API, a code reviewer would likely catch that. LogMessage() previously did not have this property -- I could easily build up a complex string and pass it to LogMessage(), which would be harder for a reviewer to reason about -- did everything get sanitized properly or not? Now, in terms of a compromised renderer: First of all, if you've compromised the renderer, your next step probably isn't going to be to ask the user to enable logging, and then try to send across a password or a sensitive URL over IPC. Even if that is your goal, you can still do it, by logging the sensitive data as a "name_or_id". In other words, I think the threat model for sending sensitive data from the renderer to the browser is coding mistakes, not malicious websites. For that threat model, we can keep the cleaner option. (Of course, Tom is much more expert in this area than I am, so perhaps I am overlooking something that he sees. Corrections are definitely welcome.) On the browser side, it's still very important to sanitize the received data by escaping it (as you've done now), because there is a much higher incentive for a malicious website to try to run arbitrary code in a chrome:// page than there is to reveal a user's password.
Thanks, Ilya. In the meantime I addressed your comment about the ID->string map, as that's going to stay in any case. I keep track of the other comments you made and I left unaddressed -- once we finalize the design, I'll get back to them. Thanks a lot for your detailed reasons for sending just a string over the IPC. I understand what you say, and I'm also not clear on the threat model possibly addressed by sanitizing after the IPC lands in the browser. Tom mentioned the preference for browser-side sanitization in https://codereview.chromium.org/228263004/#msg23, but then continued that the general issue is about stopping the programming mistakes. I agree with your point that the programming mistakes should be taken care of by the restricted LogFoo methods, independent of whether called in renderer or browser. Tom, could you please help us clarify the above? Thanks, Vaclav https://codereview.chromium.org/235623002/diff/60001/components/autofill/core... File components/autofill/core/common/save_password_progress_logger.h (right): https://codereview.chromium.org/235623002/diff/60001/components/autofill/core... components/autofill/core/common/save_password_progress_logger.h:40: // file. On 2014/04/12 00:00:51, Ilya Sherman wrote: > Rather than requiring the order to match, please just use a switch stmt with > cases. It's a little longer, but it's also more robust. Done.
Compromised renderers are not the issue. The issue is a lack of auditability for the IPC and being robust against coding errors. So I'm ok with the proposal in #c10.
Thanks Tom and Ilya. I addressed all remaining comments and changed the implementation as suggested in #10. Could you both please do one more review (Tom for IPC and security, Ilya for the rest)? Thanks, Vaclav https://codereview.chromium.org/235623002/diff/60001/chrome/browser/password_... File chrome/browser/password_manager/chrome_password_manager_client.h (right): https://codereview.chromium.org/235623002/diff/60001/chrome/browser/password_... chrome/browser/password_manager/chrome_password_manager_client.h:57: autofill::SavePasswordProgressLogger::StructuredLog>& logs) OVERRIDE; Resolved: The type got shorter again. On 2014/04/12 09:11:38, vabr (Chromium) wrote: > On 2014/04/12 00:00:51, Ilya Sherman wrote: > > Wow, is this really how clang-format formats this line? That's not great :/ > > I can confirm this was clang-formated. > Have you suggestions for a better split? I actually don't see any, the type name > got monstrously long. https://codereview.chromium.org/235623002/diff/60001/components/autofill/core... File components/autofill/core/common/save_password_progress_logger.h (right): https://codereview.chromium.org/235623002/diff/60001/components/autofill/core... components/autofill/core/common/save_password_progress_logger.h:120: }; On 2014/04/12 22:25:31, Ilya Sherman wrote: > On 2014/04/12 09:11:38, vabr (Chromium) wrote: > > On 2014/04/12 00:00:51, Ilya Sherman wrote: > > > I'm not convinced that passing a struct rather than a string across the > > process > > > boundary improves security in any way. I think it's sufficient to just > > replace > > > the string "label" parameters with StringIDs instead. > > > > > > If either of you disagree, could you please convey what security or privacy > > > threat the StructuredLog class prevents, over and above the guarantees made > by > > > the LogFoo() methods below? > > > > My understanding is that LogFoo don't actually present any guarantees, as long > > as they can be called from the (untrusted) renderer. > > > > If I understand you correctly, you suggest passing a StringID for a label and > a > > string for the data through the IPC? > > If so, then it will be hardly possible to make sure on the browser side, that, > > e.g., a password or a sensitive URL is not included in the string. > > I would keep the previous IPC design, i.e. just send a string. However, I would > keep the StringID changes for the LogFoo() methods in this class. > > With this design, it's not possible to write code that accidentally emits a > password or a sensitive URL, other than by going through some contortions to, > say, pass the password as the name_or_id param to LogHTMLForm(). That is, > there's no chance that I as a developer might add code to log a password, unless > I really abuse the API... but if I'm really abusing the API, a code reviewer > would likely catch that. LogMessage() previously did not have this property -- > I could easily build up a complex string and pass it to LogMessage(), which > would be harder for a reviewer to reason about -- did everything get sanitized > properly or not? > > Now, in terms of a compromised renderer: First of all, if you've compromised the > renderer, your next step probably isn't going to be to ask the user to enable > logging, and then try to send across a password or a sensitive URL over IPC. > Even if that is your goal, you can still do it, by logging the sensitive data as > a "name_or_id". > > In other words, I think the threat model for sending sensitive data from the > renderer to the browser is coding mistakes, not malicious websites. For that > threat model, we can keep the cleaner option. (Of course, Tom is much more > expert in this area than I am, so perhaps I am overlooking something that he > sees. Corrections are definitely welcome.) > > On the browser side, it's still very important to sanitize the received data by > escaping it (as you've done now), because there is a much higher incentive for a > malicious website to try to run arbitrary code in a chrome:// page than there is > to reveal a user's password. Done. https://codereview.chromium.org/235623002/diff/60001/components/autofill/core... components/autofill/core/common/save_password_progress_logger.h:133: const std::vector<StructuredLog>& logs); On 2014/04/12 00:00:51, Ilya Sherman wrote: > How about only making this function available in the browser process? That function is gone.
lgtm
https://codereview.chromium.org/235623002/diff/170001/components/autofill/cor... File components/autofill/core/common/save_password_progress_logger.cc (right): https://codereview.chromium.org/235623002/diff/170001/components/autofill/cor... components/autofill/core/common/save_password_progress_logger.cc:33: // is not open), that optimizing for code robustness is preferred agains speed. nit: "agains" -> "against" https://codereview.chromium.org/235623002/diff/170001/components/autofill/cor... components/autofill/core/common/save_password_progress_logger.cc:96: return NULL; NULL isn't a valid std::string. I think you meant "std::string()" instead. https://codereview.chromium.org/235623002/diff/170001/components/autofill/cor... components/autofill/core/common/save_password_progress_logger.cc:187: } Hmm, I'm not thrilled about the addition of this method and all of the overloaded GetValueFromLog() methods. Overloaded methods are discouraged by the style guide, and by most reviewers in general. Why did you switch to this approach, rather than keeping the previous design? https://codereview.chromium.org/235623002/diff/170001/components/autofill/cor... File components/autofill/core/common/save_password_progress_logger_unittest.cc (left): https://codereview.chromium.org/235623002/diff/170001/components/autofill/cor... components/autofill/core/common/save_password_progress_logger_unittest.cc:143: } Hmm, why did you remove these tests? https://codereview.chromium.org/235623002/diff/170001/components/autofill/cor... File components/autofill/core/common/save_password_progress_logger_unittest.cc (right): https://codereview.chromium.org/235623002/diff/170001/components/autofill/cor... components/autofill/core/common/save_password_progress_logger_unittest.cc:88: "formname", nit: IMO "form_name" is a better name, and we should preserve hyphens and underscores as well as alphanumerics.
Thanks, Ilya, I addressed/answered all your comments, PTAL. Thanks, Tom. Based on Ilya feedback I did a very minor security change: underscore and hyphen is no longer filtered out of element ID names, see IsUnwantedInElementID(char c) in components/autofill/core/common/save_password_progress_logger.cc. Please shout if you object (otherwise no response necessary). Thanks, Vaclav https://codereview.chromium.org/235623002/diff/170001/components/autofill/cor... File components/autofill/core/common/save_password_progress_logger.cc (right): https://codereview.chromium.org/235623002/diff/170001/components/autofill/cor... components/autofill/core/common/save_password_progress_logger.cc:33: // is not open), that optimizing for code robustness is preferred agains speed. On 2014/04/16 22:02:11, Ilya Sherman wrote: > nit: "agains" -> "against" Done. https://codereview.chromium.org/235623002/diff/170001/components/autofill/cor... components/autofill/core/common/save_password_progress_logger.cc:96: return NULL; On 2014/04/16 22:02:11, Ilya Sherman wrote: > NULL isn't a valid std::string. I think you meant "std::string()" instead. Thanks! That was a relic from the time this returned const char*. Done. https://codereview.chromium.org/235623002/diff/170001/components/autofill/cor... components/autofill/core/common/save_password_progress_logger.cc:187: } On 2014/04/16 22:02:11, Ilya Sherman wrote: > Hmm, I'm not thrilled about the addition of this method and all of the > overloaded GetValueFromLog() methods. Overloaded methods are discouraged by the > style guide, and by most reviewers in general. As for the style guide, it says: "Use overloaded functions (including constructors) only if a reader looking at a call site can get a good idea of what is happening without having to first figure out exactly which overload is being called." I understood it as: so long the function does the same thing (here: convert some input to a base::Value), it should be OK to name it the same for multiple type-dependent implementations. But I acknowledge that the cited piece of the guide allows wide interpretations. > Why did you switch to this > approach, rather than keeping the previous design? My goal was to make the inside of LogPasswordForm readable. That means that the code adding a value to the dictionary should clearly display what values are added, as opposed to "GetStringFromID" boilerplate. I will do it without the template and overloading, at the cost of having different copies of AddToDict, but also at the benefit of knowing the data type straight from the Add* method name. Both is acceptable to me, please let me know if the new code looks better to you. Note: I still kept one instance of overloading: AddElementIDToDict for std::string and base::string16. Overloading for 8- and 16-bit versions of strings seems to be accepted in the codebase. If you disagree, just let me know and I'll split the name here as well. https://codereview.chromium.org/235623002/diff/170001/components/autofill/cor... File components/autofill/core/common/save_password_progress_logger_unittest.cc (left): https://codereview.chromium.org/235623002/diff/170001/components/autofill/cor... components/autofill/core/common/save_password_progress_logger_unittest.cc:143: } On 2014/04/16 22:02:11, Ilya Sherman wrote: > Hmm, why did you remove these tests? Because the LogFinalDecision() methods no longer exists. It is replaced by LogMessage with the appropriate StringID. https://codereview.chromium.org/235623002/diff/170001/components/autofill/cor... File components/autofill/core/common/save_password_progress_logger_unittest.cc (right): https://codereview.chromium.org/235623002/diff/170001/components/autofill/cor... components/autofill/core/common/save_password_progress_logger_unittest.cc:88: "formname", On 2014/04/16 22:02:11, Ilya Sherman wrote: > nit: IMO "form_name" is a better name, and we should preserve hyphens and > underscores as well as alphanumerics. Done.
LGTM, but please consider my comment below: https://codereview.chromium.org/235623002/diff/210001/components/autofill/cor... File components/autofill/core/common/save_password_progress_logger.cc (right): https://codereview.chromium.org/235623002/diff/210001/components/autofill/cor... components/autofill/core/common/save_password_progress_logger.cc:228: &log, STRING_SCHEME_MESSAGE, FormSchemeToStringID(form.scheme)); Comparing AddMessageToDict( &log, STRING_SCHEME_MESSAGE, FormSchemeToStringID(form.scheme)); vs. log.SetString(GetStringFromID(STRING_SCHEME_MESSAGE), FormSchemeToString(form.scheme)); I actually think the second is more readable, because it's clear to me from the call site exactly what's happening: A single element is being added to the dictionary. With AddMessageToDict(), I have to look at that method to see that it's just setting a single field. Plus, the Add*ToDict and GetValueFrom* methods add something like 60 lines of boilerplate. All that said, this is a fairly minor style issue. If you really don't find my reasoning convincing, then I won't block this CL over this disagreement.
Thanks Ilya, For all the helpful comments. I believe it was straightforward to address your last one, so sending to CQ given your approval. Please do not hesitate to let me know if you want me to do further changes, I'll address them in a separate CL. Cheers, Vaclav https://codereview.chromium.org/235623002/diff/210001/components/autofill/cor... File components/autofill/core/common/save_password_progress_logger.cc (right): https://codereview.chromium.org/235623002/diff/210001/components/autofill/cor... components/autofill/core/common/save_password_progress_logger.cc:228: &log, STRING_SCHEME_MESSAGE, FormSchemeToStringID(form.scheme)); On 2014/04/17 21:12:05, Ilya Sherman wrote: > Comparing > > AddMessageToDict( > &log, STRING_SCHEME_MESSAGE, FormSchemeToStringID(form.scheme)); > > vs. > > log.SetString(GetStringFromID(STRING_SCHEME_MESSAGE), > FormSchemeToString(form.scheme)); > > I actually think the second is more readable, because it's clear to me from the > call site exactly what's happening: A single element is being added to the > dictionary. With AddMessageToDict(), I have to look at that method to see that > it's just setting a single field. Plus, the Add*ToDict and GetValueFrom* > methods add something like 60 lines of boilerplate. > > All that said, this is a fairly minor style issue. If you really don't find my > reasoning convincing, then I won't block this CL over this disagreement. It was, in fact, very convincing. Happy to remove the unnecessary helper methods. Thanks!
The CQ bit was checked by vabr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vabr@chromium.org/235623002/250001
Thanks! :) (Still) LGTM, with a final couple of nits: https://codereview.chromium.org/235623002/diff/250001/components/autofill/cor... File components/autofill/core/common/save_password_progress_logger.cc (right): https://codereview.chromium.org/235623002/diff/250001/components/autofill/cor... components/autofill/core/common/save_password_progress_logger.cc:11: #include "base/memory/scoped_ptr.h" nit: Not needed? https://codereview.chromium.org/235623002/diff/250001/components/autofill/cor... components/autofill/core/common/save_password_progress_logger.cc:123: std::string ScrubElementID(base::string16 element_id) { nit: Pass by const-reference.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_rel
The CQ bit was checked by vabr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vabr@chromium.org/235623002/250001
The CQ bit was unchecked by vabr@chromium.org
Thanks Ilya, Your most recent comments are now addressed. Sorry for ignoring them earlier, that was fully unintentional, don't know how I missed your update at the first time. The revised CL is uploaded, so I'm sending back to the CQ. Cheers, Vaclav https://codereview.chromium.org/235623002/diff/250001/components/autofill/cor... File components/autofill/core/common/save_password_progress_logger.cc (right): https://codereview.chromium.org/235623002/diff/250001/components/autofill/cor... components/autofill/core/common/save_password_progress_logger.cc:11: #include "base/memory/scoped_ptr.h" On 2014/04/19 07:51:22, Ilya Sherman wrote: > nit: Not needed? Done. https://codereview.chromium.org/235623002/diff/250001/components/autofill/cor... components/autofill/core/common/save_password_progress_logger.cc:123: std::string ScrubElementID(base::string16 element_id) { On 2014/04/19 07:51:22, Ilya Sherman wrote: > nit: Pass by const-reference. Thanks! This was a silly copy-paste from the 8-bit string version, where I pass by value on purpose (the argument is used as a local variable transformed by the replace_if algorithm). Done.
The CQ bit was checked by vabr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vabr@chromium.org/235623002/270001
Message was sent while issue was closed.
Change committed as 264949
Message was sent while issue was closed.
On 2014/04/12 09:11:37, vabr (OOO until late Feb) wrote: > Thanks Tom and Ilya, for your comments. > > I have yet to answer a couple of Ilya's comments, but we need to reach an > understanding first on what should be sent over the IPC. > > Tom, it would be great if you could chime in to the discussion about the format > of the IPC message below, as any changes to that would need a re-review by you > anyway. > > Thanks, > Vaclav > > https://codereview.chromium.org/235623002/diff/60001/chrome/browser/password_... > File chrome/browser/password_manager/chrome_password_manager_client.h (right): > > https://codereview.chromium.org/235623002/diff/60001/chrome/browser/password_... > chrome/browser/password_manager/chrome_password_manager_client.h:57: > autofill::SavePasswordProgressLogger::StructuredLog>& logs) OVERRIDE; > On 2014/04/12 00:00:51, Ilya Sherman wrote: > > Wow, is this really how clang-format formats this line? That's not great :/ > > I can confirm this was clang-formated. > Have you suggestions for a better split? I actually don't see any, the type name > got monstrously long. > > https://codereview.chromium.org/235623002/diff/60001/components/autofill/core... > File components/autofill/core/common/save_password_progress_logger.h (right): > > https://codereview.chromium.org/235623002/diff/60001/components/autofill/core... > components/autofill/core/common/save_password_progress_logger.h:120: }; > On 2014/04/12 00:00:51, Ilya Sherman wrote: > > I'm not convinced that passing a struct rather than a string across the > process > > boundary improves security in any way. I think it's sufficient to just > replace > > the string "label" parameters with StringIDs instead. > > > > If either of you disagree, could you please convey what security or privacy > > threat the StructuredLog class prevents, over and above the guarantees made by > > the LogFoo() methods below? > > My understanding is that LogFoo don't actually present any guarantees, as long > as they can be called from the (untrusted) renderer. > > If I understand you correctly, you suggest passing a StringID for a label and a > string for the data through the IPC? > If so, then it will be hardly possible to make sure on the browser side, that, > e.g., a password or a sensitive URL is not included in the string. my whole chrome book crashed says no system i need a USB to fix the hacker wiped out my computers so I having a hard time. this problems are also in our phones and tablets I don know what to do . please someone help!!!!
Message was sent while issue was closed.
On 2014/04/12 09:11:37, vabr (OOO until late Feb) wrote: > Thanks Tom and Ilya, for your comments. > > I have yet to answer a couple of Ilya's comments, but we need to reach an > understanding first on what should be sent over the IPC. > > Tom, it would be great if you could chime in to the discussion about the format > of the IPC message below, as any changes to that would need a re-review by you > anyway. > > Thanks, > Vaclav > > https://codereview.chromium.org/235623002/diff/60001/chrome/browser/password_... > File chrome/browser/password_manager/chrome_password_manager_client.h (right): > > https://codereview.chromium.org/235623002/diff/60001/chrome/browser/password_... > chrome/browser/password_manager/chrome_password_manager_client.h:57: > autofill::SavePasswordProgressLogger::StructuredLog>& logs) OVERRIDE; > On 2014/04/12 00:00:51, Ilya Sherman wrote: > > Wow, is this really how clang-format formats this line? That's not great :/ > > I can confirm this was clang-formated. > Have you suggestions for a better split? I actually don't see any, the type name > got monstrously long. > > https://codereview.chromium.org/235623002/diff/60001/components/autofill/core... > File components/autofill/core/common/save_password_progress_logger.h (right): > > https://codereview.chromium.org/235623002/diff/60001/components/autofill/core... > components/autofill/core/common/save_password_progress_logger.h:120: }; > On 2014/04/12 00:00:51, Ilya Sherman wrote: > > I'm not convinced that passing a struct rather than a string across the > process > > boundary improves security in any way. I think it's sufficient to just > replace > > the string "label" parameters with StringIDs instead. > > > > If either of you disagree, could you please convey what security or privacy > > threat the StructuredLog class prevents, over and above the guarantees made by > > the LogFoo() methods below? > > My understanding is that LogFoo don't actually present any guarantees, as long > as they can be called from the (untrusted) renderer. > > If I understand you correctly, you suggest passing a StringID for a label and a > string for the data through the IPC? > If so, then it will be hardly possible to make sure on the browser side, that, > e.g., a password or a sensitive URL is not included in the string. my whole chrome book crashed says no system i need a USB to fix the hacker wiped out my computers so I having a hard time. this problems are also in our phones and tablets I don know what to do . please someone help!!!!
Message was sent while issue was closed.
I am the only owner and user Jennifer Skyles 407 2721040 you have been helping my hacker!!! On Feb 13, 2017 11:34 PM, <jjr62299@gmail.com> wrote: > On 2014/04/12 09:11:37, vabr (OOO until late Feb) wrote: > > Thanks Tom and Ilya, for your comments. > > > > I have yet to answer a couple of Ilya's comments, but we need to reach an > > understanding first on what should be sent over the IPC. > > > > Tom, it would be great if you could chime in to the discussion about the > format > > of the IPC message below, as any changes to that would need a re-review > by you > > anyway. > > > > Thanks, > > Vaclav > > > > > https://codereview.chromium.org/235623002/diff/60001/ > chrome/browser/password_manager/chrome_password_manager_client.h > > File chrome/browser/password_manager/chrome_password_manager_client.h > (right): > > > > > https://codereview.chromium.org/235623002/diff/60001/ > chrome/browser/password_manager/chrome_password_manager_client.h#newcode57 > > chrome/browser/password_manager/chrome_password_manager_client.h:57: > > autofill::SavePasswordProgressLogger::StructuredLog>& logs) OVERRIDE; > > On 2014/04/12 00:00:51, Ilya Sherman wrote: > > > Wow, is this really how clang-format formats this line? That's not > great :/ > > > > I can confirm this was clang-formated. > > Have you suggestions for a better split? I actually don't see any, the > type > name > > got monstrously long. > > > > > https://codereview.chromium.org/235623002/diff/60001/ > components/autofill/core/common/save_password_progress_logger.h > > File components/autofill/core/common/save_password_progress_logger.h > (right): > > > > > https://codereview.chromium.org/235623002/diff/60001/ > components/autofill/core/common/save_password_progress_logger.h#newcode120 > > components/autofill/core/common/save_password_progress_logger.h:120: }; > > On 2014/04/12 00:00:51, Ilya Sherman wrote: > > > I'm not convinced that passing a struct rather than a string across the > > process > > > boundary improves security in any way. I think it's sufficient to just > > replace > > > the string "label" parameters with StringIDs instead. > > > > > > If either of you disagree, could you please convey what security or > privacy > > > threat the StructuredLog class prevents, over and above the guarantees > made > by > > > the LogFoo() methods below? > > > > My understanding is that LogFoo don't actually present any guarantees, > as long > > as they can be called from the (untrusted) renderer. > > > > If I understand you correctly, you suggest passing a StringID for a > label and > a > > string for the data through the IPC? > > If so, then it will be hardly possible to make sure on the browser side, > that, > > e.g., a password or a sensitive URL is not included in the string. > > my whole chrome book crashed says no system i need a USB to fix the hacker > wiped > out my computers so I having a hard time. this problems are also in our > phones > and tablets I don know what to do . please someone help!!!! > > https://codereview.chromium.org/235623002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Jennifer is the only owner 407 272 1040 you have Brent helping my hacker On Feb 13, 2017 11:34 PM, <jjr62299@gmail.com> wrote: > On 2014/04/12 09:11:37, vabr (OOO until late Feb) wrote: > > Thanks Tom and Ilya, for your comments. > > > > I have yet to answer a couple of Ilya's comments, but we need to reach an > > understanding first on what should be sent over the IPC. > > > > Tom, it would be great if you could chime in to the discussion about the > format > > of the IPC message below, as any changes to that would need a re-review > by you > > anyway. > > > > Thanks, > > Vaclav > > > > > https://codereview.chromium.org/235623002/diff/60001/ > chrome/browser/password_manager/chrome_password_manager_client.h > > File chrome/browser/password_manager/chrome_password_manager_client.h > (right): > > > > > https://codereview.chromium.org/235623002/diff/60001/ > chrome/browser/password_manager/chrome_password_manager_client.h#newcode57 > > chrome/browser/password_manager/chrome_password_manager_client.h:57: > > autofill::SavePasswordProgressLogger::StructuredLog>& logs) OVERRIDE; > > On 2014/04/12 00:00:51, Ilya Sherman wrote: > > > Wow, is this really how clang-format formats this line? That's not > great :/ > > > > I can confirm this was clang-formated. > > Have you suggestions for a better split? I actually don't see any, the > type > name > > got monstrously long. > > > > > https://codereview.chromium.org/235623002/diff/60001/ > components/autofill/core/common/save_password_progress_logger.h > > File components/autofill/core/common/save_password_progress_logger.h > (right): > > > > > https://codereview.chromium.org/235623002/diff/60001/ > components/autofill/core/common/save_password_progress_logger.h#newcode120 > > components/autofill/core/common/save_password_progress_logger.h:120: }; > > On 2014/04/12 00:00:51, Ilya Sherman wrote: > > > I'm not convinced that passing a struct rather than a string across the > > process > > > boundary improves security in any way. I think it's sufficient to just > > replace > > > the string "label" parameters with StringIDs instead. > > > > > > If either of you disagree, could you please convey what security or > privacy > > > threat the StructuredLog class prevents, over and above the guarantees > made > by > > > the LogFoo() methods below? > > > > My understanding is that LogFoo don't actually present any guarantees, > as long > > as they can be called from the (untrusted) renderer. > > > > If I understand you correctly, you suggest passing a StringID for a > label and > a > > string for the data through the IPC? > > If so, then it will be hardly possible to make sure on the browser side, > that, > > e.g., a password or a sensitive URL is not included in the string. > > my whole chrome book crashed says no system i need a USB to fix the hacker > wiped > out my computers so I having a hard time. this problems are also in our > phones > and tablets I don know what to do . please someone help!!!! > > https://codereview.chromium.org/235623002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2017/02/14 05:39:04, js33 wrote: > Jennifer is the only owner 407 272 1040 you have Brent helping my hacker > > On Feb 13, 2017 11:34 PM, <mailto:jjr62299@gmail.com> wrote: > > > On 2014/04/12 09:11:37, vabr (OOO until late Feb) wrote: > > > Thanks Tom and Ilya, for your comments. > > > > > > I have yet to answer a couple of Ilya's comments, but we need to reach an > > > understanding first on what should be sent over the IPC. > > > > > > Tom, it would be great if you could chime in to the discussion about the > > format > > > of the IPC message below, as any changes to that would need a re-review > > by you > > > anyway. > > > > > > Thanks, > > > Vaclav > > > > > > > > https://codereview.chromium.org/235623002/diff/60001/ > > chrome/browser/password_manager/chrome_password_manager_client.h > > > File chrome/browser/password_manager/chrome_password_manager_client.h > > (right): > > > > > > > > https://codereview.chromium.org/235623002/diff/60001/ > > chrome/browser/password_manager/chrome_password_manager_client.h#newcode57 > > > chrome/browser/password_manager/chrome_password_manager_client.h:57: > > > autofill::SavePasswordProgressLogger::StructuredLog>& logs) OVERRIDE; > > > On 2014/04/12 00:00:51, Ilya Sherman wrote: > > > > Wow, is this really how clang-format formats this line? That's not > > great :/ > > > > > > I can confirm this was clang-formated. > > > Have you suggestions for a better split? I actually don't see any, the > > type > > name > > > got monstrously long. > > > > > > > > https://codereview.chromium.org/235623002/diff/60001/ > > components/autofill/core/common/save_password_progress_logger.h > > > File components/autofill/core/common/save_password_progress_logger.h > > (right): > > > > > > > > https://codereview.chromium.org/235623002/diff/60001/ > > components/autofill/core/common/save_password_progress_logger.h#newcode120 > > > components/autofill/core/common/save_password_progress_logger.h:120: }; > > > On 2014/04/12 00:00:51, Ilya Sherman wrote: > > > > I'm not convinced that passing a struct rather than a string across the > > > process > > > > boundary improves security in any way. I think it's sufficient to just > > > replace > > > > the string "label" parameters with StringIDs instead. > > > > > > > > If either of you disagree, could you please convey what security or > > privacy > > > > threat the StructuredLog class prevents, over and above the guarantees > > made > > by > > > > the LogFoo() methods below? > > > > > > My understanding is that LogFoo don't actually present any guarantees, > > as long > > > as they can be called from the (untrusted) renderer. > > > > > > If I understand you correctly, you suggest passing a StringID for a > > label and > > a > > > string for the data through the IPC? > > > If so, then it will be hardly possible to make sure on the browser side, > > that, > > > e.g., a password or a sensitive URL is not included in the string. > > > > my whole chrome book crashed says no system i need a USB to fix the hacker > > wiped > > out my computers so I having a hard time. this problems are also in our > > phones > > and tablets I don know what to do . please someone help!!!! > > > > https://codereview.chromium.org/235623002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Jennifer is the only owner you have been helping my hacker! 4072721040
Message was sent while issue was closed.
do what ever you gotta do i think vabr+owner is my hacker if so can we track him the FBI is already involved with all the documentation i found On Mon, Feb 13, 2017 at 11:34 PM, <jjr62299@gmail.com> wrote: > On 2014/04/12 09:11:37, vabr (OOO until late Feb) wrote: > > Thanks Tom and Ilya, for your comments. > > > > I have yet to answer a couple of Ilya's comments, but we need to reach an > > understanding first on what should be sent over the IPC. > > > > Tom, it would be great if you could chime in to the discussion about the > format > > of the IPC message below, as any changes to that would need a re-review > by you > > anyway. > > > > Thanks, > > Vaclav > > > > > https://codereview.chromium.org/235623002/diff/60001/ > chrome/browser/password_manager/chrome_password_manager_client.h > > File chrome/browser/password_manager/chrome_password_manager_client.h > (right): > > > > > https://codereview.chromium.org/235623002/diff/60001/ > chrome/browser/password_manager/chrome_password_manager_client.h#newcode57 > > chrome/browser/password_manager/chrome_password_manager_client.h:57: > > autofill::SavePasswordProgressLogger::StructuredLog>& logs) OVERRIDE; > > On 2014/04/12 00:00:51, Ilya Sherman wrote: > > > Wow, is this really how clang-format formats this line? That's not > great :/ > > > > I can confirm this was clang-formated. > > Have you suggestions for a better split? I actually don't see any, the > type > name > > got monstrously long. > > > > > https://codereview.chromium.org/235623002/diff/60001/ > components/autofill/core/common/save_password_progress_logger.h > > File components/autofill/core/common/save_password_progress_logger.h > (right): > > > > > https://codereview.chromium.org/235623002/diff/60001/ > components/autofill/core/common/save_password_progress_logger.h#newcode120 > > components/autofill/core/common/save_password_progress_logger.h:120: }; > > On 2014/04/12 00:00:51, Ilya Sherman wrote: > > > I'm not convinced that passing a struct rather than a string across the > > process > > > boundary improves security in any way. I think it's sufficient to just > > replace > > > the string "label" parameters with StringIDs instead. > > > > > > If either of you disagree, could you please convey what security or > privacy > > > threat the StructuredLog class prevents, over and above the guarantees > made > by > > > the LogFoo() methods below? > > > > My understanding is that LogFoo don't actually present any guarantees, > as long > > as they can be called from the (untrusted) renderer. > > > > If I understand you correctly, you suggest passing a StringID for a > label and > a > > string for the data through the IPC? > > If so, then it will be hardly possible to make sure on the browser side, > that, > > e.g., a password or a sensitive URL is not included in the string. > > my whole chrome book crashed says no system i need a USB to fix the hacker > wiped > out my computers so I having a hard time. this problems are also in our > phones > and tablets I don know what to do . please someone help!!!! > > https://codereview.chromium.org/235623002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
