{"action":"created","issue":{"url":"https://api.github.com/repos/OWASP/passfault/issues/70","repository_url":"https://api.github.com/repos/OWASP/passfault","labels_url":"https://api.github.com/repos/OWASP/passfault/issues/70/labels{/name}","comments_url":"https://api.github.com/repos/OWASP/passfault/issues/70/comments","events_url":"https://api.github.com/repos/OWASP/passfault/issues/70/events","html_url":"https://github.com/OWASP/passfault/issues/70","id":339141566,"node_id":"MDU6SXNzdWUzMzkxNDE1NjY=","number":70,"title":"SecureString Thread-locale cache","user":{"login":"shathor","id":10561355,"node_id":"MDQ6VXNlcjEwNTYxMzU1","avatar_url":"https://avatars1.githubusercontent.com/u/10561355?v=4","gravatar_id":"","url":"https://api.github.com/users/shathor","html_url":"https://github.com/shathor","followers_url":"https://api.github.com/users/shathor/followers","following_url":"https://api.github.com/users/shathor/following{/other_user}","gists_url":"https://api.github.com/users/shathor/gists{/gist_id}","starred_url":"https://api.github.com/users/shathor/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/shathor/subscriptions","organizations_url":"https://api.github.com/users/shathor/orgs","repos_url":"https://api.github.com/users/shathor/repos","events_url":"https://api.github.com/users/shathor/events{/privacy}","received_events_url":"https://api.github.com/users/shathor/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":0,"created_at":"2018-07-07T12:12:19Z","updated_at":"2018-07-09T15:35:10Z","closed_at":null,"author_association":"NONE","body":"Looking at the [SecureString ](https://github.com/OWASP/passfault/blob/master/core/src/main/java/org/owasp/passfault/impl/SecureString.jav )implementation, wouldn't it be better to put a synchronized block on the chars-array around the `Arrays.fill` call?\r\n\r\nE.g.\r\n```\r\npublic void clear(){\r\n    synchronized(chars) {\r\n        Arrays.fill(chars, '0');\r\n    }\r\n}\r\n```\r\n\r\n=> This would give assurance, that the JVM doesn't optimize anything around fill and prevent thread-local caching (as per this [thread](https://www.reddit.com/r/java/comments/8wrgzx/java_equivalent_of_securestring/e1xvwnd/))\r\n\r\nActually you could make this class thread-safe while at it, with read-locks for all other methods and write-lock for the clear().\r\n\r\nAdditionally it's not clear, that the class creates a copy of the input char-array. It's likely users forget to clear their input \"manually\" after creating an instance.\r\n\r\nIf you want I can make a PR."},"comment":{"url":"https://api.github.com/repos/OWASP/passfault/issues/comments/403521498","html_url":"https://github.com/OWASP/passfault/issues/70#issuecomment-403521498","issue_url":"https://api.github.com/repos/OWASP/passfault/issues/70","id":403521498,"node_id":"MDEyOklzc3VlQ29tbWVudDQwMzUyMTQ5OA==","user":{"login":"c-a-m","id":1190407,"node_id":"MDQ6VXNlcjExOTA0MDc=","avatar_url":"https://avatars1.githubusercontent.com/u/1190407?v=4","gravatar_id":"","url":"https://api.github.com/users/c-a-m","html_url":"https://github.com/c-a-m","followers_url":"https://api.github.com/users/c-a-m/followers","following_url":"https://api.github.com/users/c-a-m/following{/other_user}","gists_url":"https://api.github.com/users/c-a-m/gists{/gist_id}","starred_url":"https://api.github.com/users/c-a-m/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/c-a-m/subscriptions","organizations_url":"https://api.github.com/users/c-a-m/orgs","repos_url":"https://api.github.com/users/c-a-m/repos","events_url":"https://api.github.com/users/c-a-m/events{/privacy}","received_events_url":"https://api.github.com/users/c-a-m/received_events","type":"User","site_admin":false},"created_at":"2018-07-09T15:35:10Z","updated_at":"2018-07-09T15:35:10Z","author_association":"COLLABORATOR","body":"I'm torn.  While I do want bullet proof code, the intent of this class is that it be mostly immutable - except for when we are done using it.  When we are done analyzing it, passwords should be cleared out and no longer in memory.  At that point passfault should be done using an instance of this object - so thread safety shouldn't be a concern.  So I'd hate to slow down access to SecureString by making it thread-safe. \r\n\r\nOn copying the array, that's a great point.  In fact I bet I do that same thing in the servlet code.  Yep, here it is: https://github.com/OWASP/passfault/blob/master/jsonService/src/main/java/org/owasp/passfault/web/PassfaultServlet.java    Nice find! "}}
{"actor":{"display_login":"c-a-m"}}